Page MenuHomePhabricator

[DWARF] Handle DW_OP_entry_value operand
ClosedPublic

Authored by djtodoro on Apr 18 2019, 6:13 AM.

Details

Summary

IR and AsmPrinter parts for handling of DW_OP_entry_values DWARF operation.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Repository
rL LLVM

Event Timeline

djtodoro created this revision.Apr 18 2019, 6:13 AM

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

Thanks!

an entry in SourceLevelDebugging.rst (or LangRef.rst?) explaining the semantics and limitations of our DW_OP_entry_value support

Done.

a Verifier check that yells when a DW_OP_entry_value is not at position 0

Done.

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. :)

djtodoro updated this revision to Diff 196429.Apr 24 2019, 5:37 AM
djtodoro added a subscriber: petarj.

-Add documentation
-Add a verifier check

aprantl added inline comments.Apr 24 2019, 1:08 PM
docs/LangRef.rst
4697 ↗(On Diff #196429)

How about:

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.
4700 ↗(On Diff #196429)

The Verifier must enforce this.

aprantl added inline comments.Apr 24 2019, 1:10 PM
lib/IR/DebugInfoMetadata.cpp
880 ↗(On Diff #196429)

Also, we must enforce the "simple register location" property.

djtodoro marked 2 inline comments as done.Apr 25 2019, 8:25 AM

@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.

docs/LangRef.rst
4697 ↗(On Diff #196429)

This?

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.

4700 ↗(On Diff #196429)

I think that it is hard to verify it at this place. Maybe when printing DWARF expression ?

aprantl added inline comments.Apr 25 2019, 9:10 AM
docs/LangRef.rst
4697 ↗(On Diff #196429)

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:

  1. we need to also document which DW_OP_reg operations are supported
  2. verify that they only appear after a DW_OP_entry_value
  3. We need to document what the value of the first argument of DW_OP_entry_value is
4700 ↗(On Diff #196429)

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?

dstenb added a subscriber: dstenb.Apr 29 2019, 8:38 AM
dstenb added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.cpp
354 ↗(On Diff #196429)

This appears to be an unintentional change (which does not compile).

djtodoro marked 3 inline comments as done.May 6 2019, 3:35 AM
djtodoro added inline comments.
docs/LangRef.rst
4697 ↗(On Diff #196429)

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.

4700 ↗(On Diff #196429)

Entry value DIExpression can have only DW_OP_entry_value for now.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
354 ↗(On Diff #196429)

Sure. The mistake was annul with the last patch from the stack. We will update this.

aprantl added inline comments.May 7 2019, 9:47 AM
docs/LangRef.rst
4697 ↗(On Diff #196429)

Perhaps a reasonable restriction (that we could enforce in the Verifier) would be that DW_OP_reg is only allowed inside a DW_OP_entry_value?

djtodoro marked an inline comment as done.May 13 2019, 1:45 AM
djtodoro added inline comments.
docs/LangRef.rst
4697 ↗(On Diff #196429)

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.
Sorry for the confusion. :)

djtodoro updated this revision to Diff 199219.May 13 2019, 1:47 AM

-Refactor the code a bit
-Improve the documentation

aprantl added inline comments.May 13 2019, 10:27 AM
include/llvm/BinaryFormat/Dwarf.def
635 ↗(On Diff #199219)

This comment is fine.

636 ↗(On Diff #199219)

We don't usually include links in the comments. This can probably be found with a search anyway.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
327 ↗(On Diff #199219)

Could we make this a verifier error instead and an assertion here?

lib/CodeGen/AsmPrinter/DwarfExpression.h
127 ↗(On Diff #199219)

Can we do this as a separate NFC patch in preparation?

djtodoro marked 4 inline comments as done.May 16 2019, 5:07 AM

@aprantl As soon as NFC patches (D61943 and D62002) got accepted we will update this.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
327 ↗(On Diff #199219)

Sure.

lib/CodeGen/AsmPrinter/DwarfExpression.h
127 ↗(On Diff #199219)

Sure.

djtodoro updated this revision to Diff 199793.May 16 2019, 5:09 AM

-Address suggestions
-Improve the documentation
-Improve the verifier

aprantl added inline comments.May 16 2019, 3:36 PM
docs/LangRef.rst
4700 ↗(On Diff #199793)

... may only appear after the ... pass.

4701 ↗(On Diff #199793)

LLVM only supports ...

aprantl added inline comments.May 16 2019, 3:37 PM
lib/IR/DebugInfoMetadata.cpp
883 ↗(On Diff #199793)

Why is it not necessary to check DW_OP_reg here?

djtodoro marked an inline comment as done.May 17 2019, 6:36 AM
djtodoro added inline comments.
lib/IR/DebugInfoMetadata.cpp
883 ↗(On Diff #199793)

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.

djtodoro updated this revision to Diff 200034.May 17 2019, 6:37 AM

-Address suggestions

aprantl added inline comments.May 17 2019, 9:38 AM
lib/IR/DebugInfoMetadata.cpp
883 ↗(On Diff #199793)

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)

djtodoro updated this revision to Diff 200488.May 21 2019, 6:40 AM
djtodoro marked an inline comment as done.

-Improve the verifier for entry values
-Add test for the verifier

aprantl added inline comments.May 21 2019, 11:21 AM
include/llvm/IR/DebugInfoMetadata.h
2542 ↗(On Diff #200488)

Note to self: this is being refactored in another review.

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
44 ↗(On Diff #200488)

If "the" location is "an" entry value

lib/DebugInfo/DWARF/DWARFExpression.cpp
278 ↗(On Diff #200488)

An llvm-dwarfdump test for this would be nice, too.

test/Verifier/diexpression-entry-value.ll
3 ↗(On Diff #200488)

, !1, !2

5 ↗(On Diff #200488)

!1 = !DIExpression(DW_OP_constu, 0, DW_OP_entry_value, 1, DW_OP_constu, 0)
!2 = !DIExpression(DW_OP_entry_value, 100, DW_OP_constu, 0)

and then perhaps also a positive test

djtodoro updated this revision to Diff 200955.May 23 2019, 6:22 AM

-Addressing comments
-Add llvm-dwarfdump tests for DW_OP_entry_value

TODO: Rebase this on top of NFCs D61943 and D62002

djtodoro updated this revision to Diff 201512.May 27 2019, 5:18 AM
djtodoro edited the summary of this revision. (Show Details)

-Refactor the code a bit
-Rebase

aprantl accepted this revision.May 28 2019, 9:24 AM
aprantl added inline comments.
include/llvm/IR/DebugInfoMetadata.h
2555 ↗(On Diff #201512)
/// Check if the expression consists of exactly one entry value operand.
/// (This is the only configuration of entry values that is supported.)
This revision is now accepted and ready to land.May 28 2019, 9:24 AM
djtodoro updated this revision to Diff 201878.May 29 2019, 5:45 AM

-Addressing suggestions

Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 20, 6:35 AM
This revision was automatically updated to reflect the committed changes.