This looks mostly good, IMO what's missing is
- an entry in SourceLevelDebugging.rst (or LangRef.rst?) explaining the semantics and limitations of our DW_OP_entry_value support
- a Verifier check that yells when a DW_OP_entry_value is not at position 0
- bonus points for refactoring the With* parameters to a flag enum that is slightly less error-prone to use.
This looks mostly good, IMO what's missing is
an entry in SourceLevelDebugging.rst (or LangRef.rst?) explaining the semantics and limitations of our DW_OP_entry_value support
a Verifier check that yells when a DW_OP_entry_value is not at position 0
bonus points for refactoring the With* parameters to a flag enum that is slightly less error-prone to use.
Since there are a lot of places using With* it is better doing this as a separate commit. I am working on the patch. :)
If an expression is marked with ``DW_OP_entry_value `` all register and memory read operations refer to the respective value at the function entry. ``DW_OP_entry_value `` may only appear after instruction selection. At this point LLVM only supports entry values for function parameters that are unmodified throughout a function and that are described as simple register location descriptions.
The Verifier must enforce this.
@aprantl Thanks again for the comments!
Let us summarize the whole picture of the feature here. In .debug_info section DW_TAG_call_site_parameter information with its attributes allows us to print @entry value from a debugger (e.g. #1 foo (val=<optimized out>, val@entry=5)). That information we try to generate during AsmPrinter phase.
The second thing of the feature is using @entry for representing real value (DBG_VALUE) of a parameter (additional input in .debug_loc). It is done by generating additional DW_OP_entry_value in LiveDebugValues. So, it will allow us to see situations in a backtrace such as #1 foo (val=val@entry=5).
In addition, we see that GCC in some situations generate DW_OP_entry_value for representing call site parameters value (see DwarfDebug.cpp file), but that is an enhancement, not a fundamental thing.
If an expression is marked with DW_OP_entry_value all register and memory read operations refer to the respective value at the function entry. DW_OP_entry_value may appear after livedebugvalues pass. At this point LLVM only supports entry values for function parameters that are unmodified throughout a function and that are described as simple register location descriptions. DW_OP_entry_value may also appear after asembler printer phase when a call site parameter value (DW_AT_call_site_parameter_value) is representd as entry value of the parameter.
In addition, we should add documentation for DW_TAG_call_site and DW_TAG_call_site_parameter as well.
I think that it is hard to verify it at this place. Maybe when printing DWARF expression ?
typos: assembler and represented
I realized that there are more things that we need to add:
Before DW_OP_entry_value, DIExpression did not support DW_OP_reg or similar, but if I understand correctly the idea is that LiveDebugValues will now directly inject register operations into the expression. If that's so:
Can you explain why? DIExpression::isValid() walks the entire expression. When we encounter a DW_OP_entry_value, we enforce that the next arg0 elements are out of a limited set of operations. Would that work?
I think we should document usage of DW_OP_reg. It introduces a confusion here. The operand is used only at the place where we are trying to describe loaded value into parameter's forwarding register (expression of DW_AT_call_site_parameter_value used for printing @entry in debugger). So, the operand could appear in DIExpression after describeLoadedValue() called from DwarfDebug::collectCallSiteParameters(). Basically, DwarfDebug::collectCallSiteParameters() is the place where we prepare info for producing DW_TAG_call_site_parameter, but within LiveDebugValues we generate just additional entries (with DW_OP_entry_value) in .debug_loc that should try using benefits of DW_TAG_call_site_parameter debug info (it means we should try reconstructing a real value for the parameter from @entry).
After LiveDebugValues pass, entry value DIExpression could have only DW_OP_entry_value operand, but final entry value DWARF expression will be printed over a register from corresponding DBG_VALUE.
Entry value DIExpression can have only DW_OP_entry_value for now.
Sure. The mistake was annul with the last patch from the stack. We will update this.
We added a documentation for DW_OP_reg in the last patch of the stack. I think that will clarify picture here. The operand has nothing to do with this part here.
This comment is fine.
We don't usually include links in the comments. This can probably be found with a search anyway.
Could we make this a verifier error instead and an assertion here?
Can we do this as a separate NFC patch in preparation?
At this point, entry value DIExpression could have only DW_OP_entry_value and 1 as the size of following DWARF expression. Since we are supporting only simple register locations the size is always 1 for now. The assembler printer will use the expression and the register operand from the DBG_VALUE (we don't embed the register info into the DIExpression explicitly) when printing it into DWARF section. This is generated during the LiveDebugValues pass to describe normal value of the parameter through the entry value (new special kind of value).
DW_OP_reg that we introduced in the last patch has nothing to do with this part and it is not used here.
I also saw that this is in the other patch later. Thanks for clarifying!
It still looks like we should verify the length / rest of the expression instead of returning without checking the later elements? We're checking the length /argument/ of the entry value, but not that the expression actually has the promised 1 argument and nothing else.
This should also be very simple to test (cf. llvm/test/Verifier/diexpression-swap.ll)
Note to self: this is being refactored in another review.
If "the" location is "an" entry value
An llvm-dwarfdump test for this would be nice, too.
, !1, !2
!1 = !DIExpression(DW_OP_constu, 0, DW_OP_entry_value, 1, DW_OP_constu, 0)
and then perhaps also a positive test