- User Since
- Jan 16 2015, 6:33 AM (272 w, 4 d)
Wed, Mar 11
Tue, Mar 10
Hi River, I have two (somewhat related) usecases for having a FuncOp without adding it to a module:
- I would like to write an mlir::OwingFuncRef BuildFunc(...) function what takes some internal/domain specific input (e.g. IR from the front end) and returns the newly built function. I could just pass in the ModuleOp as an argument and add the FuncOp to it but returning an OwningFuncRef feels like a nicer API
- My code what builds a FuncOp might fail (e.g. due to invalid input) and in that case I want to report an error without modifying the existing ModuleOp as adding a broken function to it would make the module op broken as well. My approach for it is to create an OwningFuncRef, use it to own the FuncOp during the building process (so in case of a failure it will correctly destroy it) and then in case of success I would move the FuncOp from the OwningFuncRef to the ModuleOp.
Mar 4 2020
Add missing include
Fix code formatting
Feb 19 2020
Re-try addressing review comments
Address review comments
Feb 18 2020
Fix attribute accessor to not crash for null values
Sep 30 2019
Sep 27 2019
Sep 3 2019
Jan 2 2019
Nov 5 2018
I have no memory about the history/background of the OCaml plugin but no objection from my side.
LGTM. Thank you for removing it.
LGTM. Adding Ryan as a reviewer as he was the original implementer but I am not sure if his account (and the linked e-mail) is still active.
Jun 8 2018
BTW this can also happen if we are missing some debug info.
Jan 11 2018
I think it isn't an A/B locking issue. The problem is that 2 different thread (main thread and 1 of the DWARF indexer thread) want to lock the mutex at the same time.
Nov 9 2017
I never tried debugging Objective-C using dwo but I am pretty sure this won't fix the issue you are seeing for FindCompleteObjCDefinitionTypeForDIE correctly because this way you will index the compile unit twice (once from the main object file and once from the dwo), then create 2 CompilerType for the 2 indexed version and will start hitting random issues in expression evaluation when clang will get confused by 2 declaration for the same type. If I am not mistaken then because of this, calling Index on a Dwo file is a pretty bad idea. Instead of trying to get it work we should change it to be an assert so people don't call it by accident.
How are you end up calling SymbolFileDWARFDwo::Index? If I remember correctly you are not supposed to index a dwo file directly because without the main object file you won't have all of the necessary information.
Nov 7 2017
Nov 4 2017
I verified that D39545 will be fixing the problem on the LLDB side (previously we had no proper scoped enum support in LLDB)
This change is needed to make LLDB compatible with D39239
Fix test failures
Nov 2 2017
Oct 30 2017
2 fairly random ideas without looking into it too much:
- Can you do a diff of the debug_info dump before and after your change? Understanding what have changed should give us a pretty good clue about the issue.
- My first guess is that after your change we emit DW_TAG_enumeration_type for scoped enums (what seems to be the correct thing to do) instead of something else (not sure what) and you have to update https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp#L1512 to handle it correctly.
Oct 25 2017
Oct 10 2017
Looks good. Thanks for fixing it.
Oct 9 2017
Oct 3 2017
Looks good, thanks for fixing it.
Sep 19 2017
I don't really have an opinion if moving TaskPool to Host is a good or bad idea (haven't followed the recent layering efforts) but other then that looks good. I also tested it on Linux and it works fine.
Sep 18 2017
Aug 30 2017
Aug 25 2017
Aug 24 2017
Aug 16 2017
Note about testing: I haven't manage to come up with any good approach so far for testing because only fairly new version of dwp or llvm-dwp can generate the format I am expecting here so adding a new config for the existing test suite seems a bit infeasible and also it would increase the testing time by a non negligible time.
Hi Greg, can you take a look sometime? Thanks, Tamas
Add comment about the MIPS special case.
Aug 10 2017
Thanks for fixing it
Aug 8 2017
Aug 5 2017
I checked LLDB and it doesn't have support for base address entries neither in .debug_loc nor in .debug_loc.dwo sections and it doesn't support reading .debug_loclists or .debug_loclists.dwo entries either. I will try to add support for them but it will require some refactoring so it will take some time.
Jul 31 2017
Thanks for all of the data. Based on this I think you are using a preview version of android O what reports SDK 25 but the linker works the way an SDK 26 system linker would do.
Submitted as rL309554 to get the buildbot using ToT clang green again but if you have any comment then let me know and I will address it in a followup CL.
Jul 30 2017
Improve error handling
Jul 29 2017
Jul 27 2017
Jul 26 2017
I tried to compile the code snippet you mentioned but it doesn't put anything into the debug_types section. If I move the enum definition outside of the class then it produces debug_type section but LLDB fails back gracefully without any crash or error message. I also tried a few other code snippets and the debugging experience usually wasn't great (types of local variables were messed up) but I haven't manage to get LLDB crashing.
Jul 24 2017
Can you file a bug about this issue so we can keep track of it? Also it would be nice to include a small test case in the bug (if you have one) what demonstrates the crash as so far I only managed to see missing type information without an actual LLDB crash.
This section have been already removed from Dwarf5 so I agree that we shouldn't spend too much time adding support for it. Do you know anybody hitting this issue? Do you know why they decided to use this flag?
Jun 28 2017
Looks good with one possible question
Jun 19 2017
Jun 15 2017
May 24 2017
May 21 2017
May 19 2017
May 18 2017
May 8 2017
I few high level comments after a quick read through.
May 5 2017
Makes sense. Thank you for the explanation (I assumed homogeneous aggregate and vector are the same).
May 4 2017
I am a bit confused by the correlation between your change and commit message. In the commit message you say that 32 byte structs are passed as x8 pointers but the implementation of LoadValueFromConsecutiveGPRRegisters seems to read it out from the v0-v8 registers for vectors of up to 8 elements independently of there size. Also based on that code I have the suspicion that the first branch (where byte_size <= 16) is not actually used or necessary and also I don't see anything in the ABI documentation indicating otherwise (it would be a pretty crazy ABI if they say that if you have 4 double then passed in a single 32 byte register while if you have 8 double then passed in 8 different 32 byte registers). Can you make sure that branch is necessary (e.g. removing it breaks at least 1 test)?
May 3 2017
Personally I never really liked TaskRunner (even though I was the one implemented it) because I think it adds a lot of extra complexity for fairly little gain and thinking about it a bit more I think in most use case a more targeted solution at the call site will probably give better results. Also if we need it in the future it can always be restored based on git/svn history. Based on that I am happy to delete it if we have no use case for it at the moment.
Apr 26 2017
I am fully support deleting it (and pretty much any unused code). If we need it in the future then we will still have it in the svn history.
I looked into the history of this code once and my understanding is that Enrico added this code in https://github.com/llvm-mirror/lldb/commit/bad9753828b6e0e415e38094bb9627e41d57874c but it have never been used (at least in upstream). The original commit message also indicates this.
Apr 25 2017
Apr 24 2017
Apr 10 2017
The previous version of the data formatter was triggering for std::vector<std::allocator<bool>> as well. Jason, do you know why was it the case? Do we need that functionality because of a broken compiler version or can it be removed?
Pavel created a separate fix as https://reviews.llvm.org/D31880. Abandoning this one.