Here's the the first part of the patch: support for inline locations. Support for location lists is still coming.
- Add a field in ParmVarDecl instead of VarDecl
- Use a bit in ParmVarDeclBitfields to indicate parameter modification
- Add support for the bit in ASTReaderDecl.cpp / ASTWriterDecl.cpp
- Add test case for templates
@riccibruno Thanks for your comments!
I was under the impression that space inside VarDecl was quite constrained. Pardon the likely naive question, but: is there any way to make the representation more compact (maybe sneak a bit into ParmVarDeclBitfields)?
@vsk Thanks for the comment! Sure, it is better idea. Initially, we thought this could be used even for local variables, but since we are using it only for parameters it makes more sense.
- 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.
I am wondering about the semantics of this bit. I don't think that you can know for sure within clang whether a variable has been modified. The best you can do is know for sure that some variable has been modified, but I don't think you can prove that it has not been modified.
Thu, Feb 21
Could you please add a third testcase with two DICompileUnits, both of which have a DIExpression with a DW_OP_LLVM_convert that will use the same type and then verify that we emit the type in both CUs? This will help guard against later modifications breaking this scenario.
Oh and I think that you will also have to update the serialization code/de-serialization code in ASTReaderDecl.cpp / ASTWriterDecl.cpp. You might also have to update TreeTransform but I am less familiar with this.
If this bit is relevant to function parameters, why is getIsArgumentModified in VarDecl and not in ParamVarDecl ? What you can do is move the relevant methods to ParamVarDecl, and stash the bit in ParmVarDeclBitfields.
+ rsmith, + bricci for review.
There's a nonzero chance that this patch will break llvm/tools/dsymutil. I'll see if I can come up with a testcase for that (hopefully later today).
- 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
+ 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
I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.
Looking good, thanks!
- Adjust documentation
Mon, Feb 18
LGTM with update to testcase.
deleted LLD test as it was requested. Modified llvm-objdump test to not to use clang.
- 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