- User Since
- Nov 19 2018, 1:12 AM (13 w, 5 d)
- Added test with two CUs.
- Updated some comments.
- Removed the -generate-typed-dwarf5-expr option, will only produce DW_OP_convert for Dwarf v5, for all prior versions the mask & shift expression is used. I suggest that we forget about DW_OP_GNU_convert and debugger tunings for this review. If that is really desired it can be added later in a separate patch.
Thu, Feb 21
- updated to top of trunk to have the llvm-dwarfdump updates committed earlier today
- updated tests and made additional improvements to llvm-dwarfdump
- fixed segfault bug where we crashed on release builds but not debug builds (the allocation of DIEBaseTypeRef)
Wed, Feb 20
Added support for rudimentary verify of expressions (basically to check that a BaseTypeRef operand actually points to a DW_TAG_base_type DIE).
+ Addressed review remark (use range-based loop instead of iterators).
+ Updated tests (probably forgot during last rebase. Verified the values used now with a clean master)
I have created a review for the requested 'llvm-dwarfdump' updates (the printing part) in https://reviews.llvm.org/D58442
I think that we all are. Maybe it is because we tried some many different approaches in the same review that it is a bit convoluted now.
The issue is that a location expression (either in a direct location, or in a loclist) needs to reference a DIE.
Sounds like that DIE reference is necessarily CU-local (because we're talking about precomputing the offset - and we could only do that if it's CU-local).
Also correct. The spec says that the reference is an offset into the current CU.
We already emit other CU-local DIE references in attributes (eg: DW_AT_specification, etc) references with hardcoded 4 bytes in size- so why would it be problematic to emit this one in the same way (with a padded ULEB128 that we know will give us 4 bytes of offset to work with)?
Agree. It should be no more problematic than what is done in other places already. The 4 byte ULEB128 gives us 28 bits to encode the offset in but as reasoned in other comments there is no way that limit can be reached.
& yeah, maybe if someone wants to save us some size (at the cost of whatever computational complexity this invokes in the assembler) it could be replaced with label differences (doing label differences for all DIE references would be nifty for manual editing/mucking about with DWARF, but not a big deal).
Tue, Feb 19
Mon, Feb 18
- Did some cleanup
- Added tests
- Emit DW_OP_convert for Dwarf5
- Emit legacy shift and mask expression for Dwarf4 and lower
- Added llc option -generate-typed-dwarf5-expr to force emission regardless of targeted Dwarf version
Fri, Feb 15
Addressed the inlined location expressions.
Wed, Feb 13
NVPTX supports only DWARF2 and does not know anything about DWARF5 operations. Also, it does not support any type of the expression in the DWARF sections, except for <section_name>+-<int_offset>.
I see now that the inlined places i.e. these:
git grep addBlock.*DW_AT_location lib/CodeGen/AsmPrinter lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp: addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize()); lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp: addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize()); lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp: addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize()); lib/CodeGen/AsmPrinter/DwarfUnit.cpp: addBlock(ParamDIE, dwarf::DW_AT_location, Loc);
work quite differently and that will need some additional work.
Tue, Feb 12
Removed Label from DIE (added DenseMap to CU and pass it around). Rebased to top of trunk.
Mon, Feb 11
Addresses most of the open issues I had. The major question that remains is what to do when not targeting Dwarf v5.
Thu, Feb 7
Updated to DW_OP_LLVM_convert as suggested, did the pretty printing and updated a bunch of llvm-lit tests to cope with the newly generated labels. Now the IR format looks like this:
!DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)
There are some open issues that I would like feedback on. I will add inline comments at those locations.
Wed, Feb 6
All three of these operations expect a DW_TAG_basic_type as argument. So we could easily introduce a DW_OP_LLVM_basic_type <signedness> <bits> operation that gets expanded in AsmPrinter into DW_TAG_basic_types like you do now and thus avoid having to deal with TrackingMDRefs. This would also reduce the memory footprint of all DIExpressions that don't need a type argument.
Dropping all the metadata stuff and specifying the bit size and type encoding directly in the expression vector. e.g.
!DIExpression(DW_OP_convert, 8, 5, DW_OP_convert, 32, 5, DW_OP_stack_value)
This approach certainly has its advantages and require far less changes to existing code.
Tue, Feb 5
Dwarf5 DW_OP_convert support.
If you choose this path, one way to solve the issue of what to do with the dangling types that are produced by the conversion operations would be to create them in a separate DICompileUnit with a compiler-generated name and location. This way the types should get uniqued automatically during LTO, and you don't need to worry about llvm-link & friends.
Mon, Feb 4
the type reference should be a metadata operand. Otherwise you'll need to implement support in llvm-link etc, too.
I would just implement the type reference as normal metadata operands in DIExpression and bake the knowledge that DW_OP_convert consumes one of the metadata operands into the DIExpression operand iterator.
Fri, Feb 1
Time to switch the approach. This time try adding support for typed Dwarf5 stack ops such as DW_OP_convert.
Mon, Jan 28
Insert DW_OP_deref_size in Prologue Epilogue Inserter.
Thu, Jan 24
Jan 21 2019
With the fragment approach we still have problems in e.g. lib/CodeGen/PrologEpilogInserter.cpp:1105 where the stack location of a function argument is prepended to the expression and a sized DW_OP_deref would need to be inserted.
Jan 17 2019
Uploading a prototype implementation using the DW_OP_LLVM_fragment as @bjope described. There are still some issues that need to be worked out with this so it is not final but rather I would like some input on if going down this track would be considered an acceptable solution. If there are clear objections to this approach it would be good to hear them early.
Jan 15 2019
Jan 14 2019
Unfortunately I think that there are additional complications in that our sext/zext computations will have to also depend on the endianness of the target and if the value is stored in memory or not (as having the value loaded as the 'wrong type' would require bytes to be swapped on a big endian target before it could be sext/zext by this expression).
Jan 13 2019
Jan 11 2019
Jan 3 2019
Jan 2 2019
Jan 1 2019
Dec 27 2018
Dec 21 2018
Dec 7 2018
Dec 6 2018
Dec 5 2018
Did some more experiments, trying to clean up the code and make it more generic.
Instead of only determining if X != 0 (which is clearly not sufficient) try to produce a value range for the variable and use that.
Nov 30 2018
Thanks for reviewing :)
Nov 23 2018
Nov 21 2018
I do not have a strong opinion about renaming 'transform' to 'visit' and would be happy to comply but simply doing a rename does not compile giving errors such as
Nov 20 2018
Moved ParallelLoopAccessMDKind into constructor.
Hopefully this patch solves most of the comments I have received so far.
Nov 19 2018
Split into one wrapper class for the legacy PM and one wrapper class for the new PM.
Agree that splitting would give a cleaner solution. Will give that a go.