This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Avoid adding too much indirection to pointer-valued variables
ClosedPublic

Authored by jmorse on Jun 17 2019, 7:00 AM.

Details

Summary

This patch addresses PR41675 [0], where a stack-pointer variable is dereferenced too many times by its location expression, presenting a value on the stack as the pointer to the stack.

The difference between a stack *pointer* DBG_VALUE and one that refers to a value on the stack, is currently the indirect flag. In the below, the former is the address of, the latter the value in %stack_loc:

DBG_VALUE %stack_loc, $noreg, !1, !DIExpression()
DBG_VALUE %stack_loc, 0, !1, !DIExpression()

When the prologepilog pass runs, the stack references are replaced with complex expression opcodes:

DBG_VALUE $rsp, $noreg, !1, !DIExpression(DW_OP_plus_uconst, 4)
DBG_VALUE $rsp, 0, !1, !DIExpression(DW_OP_plus_uconst, 4)

Unfortunately, the DWARF backend interprets both of these as being memory locations. For the second DBG_VALUE this is because it's explicitly indirect, but for the first it's because there's a complex expression without DW_OP_stack_value.

Get around this by detecting empty-to-non-empty DIExpression transitions in prologepilog, and adding DW_OP_stack_value to them. This ensures that pointers-to-stack-locs are interpreted as implicit locations, which is what they are.

For the edge case where the offset is zero and the location could be a register location, DIExpression::prepend will still generate opcodes, and thus DW_OP_stack_value must still be added.

[0] https://bugs.llvm.org/show_bug.cgi?id=41675

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Jun 17 2019, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 7:00 AM
aprantl added inline comments.Jun 17 2019, 10:09 AM
lib/CodeGen/PrologEpilogInserter.cpp
1191 ↗(On Diff #205062)

The only thing I'm concerned about is the DIExpr->getNumElements() == 0. This sounds like it would also reject other cases where we'd want this to apply, such as an expression that only contains a DW_OP_LLVM_fragment. Is there a better condition? Perhaps by inverting it? What's an example of an expression where we don't want this to happen?

jmorse updated this revision to Diff 205337.Jun 18 2019, 7:25 AM

Add an isComplex() method to DIExpression that detects when an expression could be interpreted as a memory location.

jmorse marked an inline comment as done.Jun 18 2019, 7:35 AM
jmorse added inline comments.
lib/CodeGen/PrologEpilogInserter.cpp
1191 ↗(On Diff #205062)

Oooofff, indeed, that definitely would cause problems. The current test that DwarfExpression makes [0] to see whether something is a simple register location or not, is whether the expression is:

  • Empty, or
  • Only a fragment

Anything else involves computing values on the expression stack: that makes it a memory location unless DW_OP_stack_value is added. I've added a method to perform that check in isComplex, and use that in PrologEpilog. I can't replace the DwarfExpression test with that method unfortunately, as it's passing around expr_operand iterators rather than DIExpressions.

[0] https://github.com/llvm/llvm-project/blob/fb9ce100d19be130d004d03088ccd4af295f3435/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp#L229

jmorse marked an inline comment as done.Jun 18 2019, 7:38 AM
jmorse added inline comments.
lib/CodeGen/PrologEpilogInserter.cpp
1191 ↗(On Diff #205062)

Hmmn, isComplex will also have to account for DW_OP_LLVM_tag_offset that's just been added. I'll need some time to pull that in and rebuild, will have that patch up shortly.

jmorse updated this revision to Diff 205360.Jun 18 2019, 8:28 AM

Account for DW_OP_LLVM_tag_offset when working out whether a DIExpression is "complex" or not.

vsk added a comment.Jun 27 2019, 3:02 PM

This looks reasonable to me. Mind adding test coverage for simple tag_offset/fragment expressions?

jmorse updated this revision to Diff 207077.Jun 28 2019, 9:37 AM

Vedant wrote:

This looks reasonable to me. Mind adding test coverage for simple tag_offset/fragment expressions?

Updated with two new variables / DBG_VALUEs to cover that -- as it turns out I was using the wrong kind of iterator in isComplex :(. Iterating elements seems to iterate over all expression storage, iterating over operands is what was needed. Great catch!

vsk accepted this revision.Jun 28 2019, 12:50 PM

Thanks!

This revision is now accepted and ready to land.Jun 28 2019, 12:50 PM
This revision was automatically updated to reflect the committed changes.