+ 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).
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
Sun, Feb 17
- Change a comment for isDescribedByReg()
- Remove a dead code
- Use isEntryValue() from DIExpression
@aprantl I just updated the diff with documentation. Please let me know what do you think.
- Add documentation for the flag
Ok. I debugged this a little and think that:
Fri, Feb 15
George, I added test for LLD. But.... It already pass for LLD without this fix. The reason for that is following quickfix :
added test for LLD. did all what was requested in comments.
Addressed the inlined location expressions.
Thu, Feb 14
Addressed most of the comments. test for LLD is in progress yet.
Hi George, Thank you for comments. I will address them. According to the questions ;
LGTM. One last request: Could you please add (either to >the doxygen comment of isArgNotModified or to >LangRef.rst or SourceLevelDebugging.rst) an >explanation of the semantics of the flag. Ie.: What it is >used for and under what conditions a frontend should >generate it?
My comments about LLD side are below.
Wed, Feb 13
David, thank you for the review. I would address all points. two major points ;
This has some relatively extensive changes to some of the llvm tools (like sancov, for instance) that aren't tested - perhaps it'd be better to leave those tools usage unmodified (adding the undef section value as a fix to the API change, but nothing more) with fixits and/or followup patches that implement the improved functionality possible with the new API, along with tests?
LGTM. One last request: Could you please add (either to the doxygen comment of isArgNotModified or to LangRef.rst or SourceLevelDebugging.rst) an explanation of the semantics of the flag. Ie.: What it is used for and under what conditions a frontend should generate it?