This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] LowerDbgDeclare: Add derefs when handling CallInst users
ClosedPublic

Authored by vsk on Jul 17 2018, 2:15 PM.

Details

Summary

LowerDbgDeclare inserts a dbg.value before each use of an address
described by a dbg.declare. When inserting a dbg.value before a CallInst
use, however, it fails to append DW_OP_deref to the DIExpression.

The DW_OP_deref is needed to reflect the fact that a dbg.value describes
a source variable directly (as opposed to a dbg.declare, which relies on
pointer indirection).

This patch adds in the DW_OP_deref where needed. This results in the
correct values being shown during a debug session for a program compiled
with ASan and optimizations (see https://reviews.llvm.org/D49520). Note
that ConvertDebugDeclareToDebugValue is already correct -- no changes
there were needed.

One complication is that SelectionDAG is unable to distinguish between
direct and indirect frame-index (FRAMEIX) SDDbgValues. This patch also
fixes this long-standing issue in order to not regress integration tests
relying on the incorrect assumption that all frame-index SDDbgValues are
indirect. This is a necessary fix: the newly-added DW_OP_derefs cannot
be lowered properly otherwise. Basically the fix prevents a direct
SDDbgValue with DIExpression(DW_OP_deref) from being dereferenced twice
by a debugger. There were a handful of tests relying on this incorrect
"FRAMEIX => indirect" assumption which actually had incorrect
DW_AT_locations: these are all fixed up in this patch.

Testing:

  • check-llvm, and an end-to-end test using lldb to debug an optimized program.
  • Existing unit tests for DIExpression::appendToStack fully cover the new DIExpression::append utility.
  • check-debuginfo (the debug info integration tests)

Diff Detail

Event Timeline

vsk created this revision.Jul 17 2018, 2:15 PM

This looks like the right change as it makes the IR more regular.
Note that in the past this worked because allocas were implicitly indirect since they caused AsmPrinter to emit a what DWARF calls a "memory location". It's possible that that code path no longer exists or isn't triggered any more.

Have you tested the clang debuginfo-tests with this change, particularly the asan and array testcases?

vsk planned changes to this revision.Jul 18 2018, 4:16 PM

This looks like the right change as it makes the IR more regular.
Note that in the past this worked because allocas were implicitly indirect since they caused AsmPrinter to emit a what DWARF calls a "memory location". It's possible that that code path no longer exists or isn't triggered any more.

Have you tested the clang debuginfo-tests with this change, particularly the asan and array testcases?

Thanks for suggesting this.

As-is, this patch breaks debuginfo-tests/stack-var.c. Without this patch we would emit a dbg.value with a concrete location:

  call void @llvm.dbg.value(metadata i32* %test, metadata !25, metadata !DIExpression()), !dbg !27
...
!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!25 = !DILocalVariable(name: "test", scope: !21, file: !1, line: 6, type: !12)

When the backend sees this, it knows to emit a DBG_VALUE which refines the concrete location:

DBG_VALUE %rbp, 0, !"test", !DIExpression(DW_OP_constu, 4, DW_OP_minus); line no:6 indirect

With this patch applied, we would instead emit:

call void @llvm.dbg.value(metadata i32* %test, metadata !25, metadata !DIExpression(DW_OP_deref)), !dbg !27

The backend sees the extra DW_OP_deref and emits a DBG_VALUE which tells the debugger to dereference a value it pulls out of the stack:

DBG_VALUE debug-use $rbp, 0, !"test", !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_deref), debug-location !27; line no:6 indirect

The problem with this patch is that it adds a DW_OP_deref without creating an implicit value to prevent that extra, incorrect dereference. The fix to this seemed simple: just add DW_OP_stack_value. That should tell the debugger not to dereference what it finds in the stack. But, that's just a more expensive way to encode the exact same behavior as a {pointer, empty DIExpression} dbg.value. Critically, it also doesn't fix my ASan test case.

Using a single DW_OP_deref (without appending DW_OP_stack_value) gives the correct location for a ASan-ified program appending to a std::deque:

(lldb) p test
(log_t) $0 = size=1 {
  [0] = (a = "toto")
}
...
Variable: id = {0x00009f25}, name = "test", type = "log_t", location =  [r15+0], decl = r376.cc:12

Note that we get the correct result (1 dereference) even though this is the *wrong* DIExpression to use, because (as an indirect value) it says to do 2 dereferences.

Adding a DW_OP_stack_value -- which fixed the stack-var.c test case -- does not fix the ASan test case. I'd expect it to cause the debugger to do 1 dereference, but instead it does 2:

(lldb) p test
error: Couldn't materialize: couldn't get the value of variable test: extracting data from value failed
...
     Variable: id = {0x00009f25}, name = "test", type = "log_t", location =  [r15+0] DW_OP_deref , decl = r376.cc:12

I think I know why adding DW_OP_stack_value fixes one test case but breaks another. My theory is that we're treating a pointer to a std::deque as a direct value (aka "implicit location"?) instead of as an indirect value (aka "concrete location"?). Here's some confirmation:

DBG_VALUE debug-use $r15, debug-use $noreg, !"test"

IIUC the fact that the second DBG_VALUE operand is $noreg means that the value is direct (implicit). The LangRef is vague about what should happen in this case, but it doesn't seem like we should get here to begin with.

rnk added a comment.Jul 19 2018, 11:15 AM

I haven't read this carefully, but this seems like the right direction. Thanks for working on it. When I last touched this, I wanted to spend time making our dbg.values of pointers unambiguous as to whether the value is the pointer or the memory the pointer refers to.

vsk updated this revision to Diff 156915.Jul 23 2018, 4:22 PM
vsk edited the summary of this revision. (Show Details)

This update fixes the long-standing issue where SelectionDAG was unable to distinguish between direct and indirect frame index debug values. Tackling this issue not only fixed the stack-var.c -O2 integration test from debuginfo-tests, but caught a slew of LLVM test cases which were checking for bogus DW_AT_locations.

rnk added a comment.Jul 23 2018, 4:49 PM

Thanks so much for seeing this through! This will finally allow us to solve int x = 0; int *px = &x;, which has been my favorite optimized debug info test case for a while now. If I filed a bug about it, I can't find it, just https://bugs.llvm.org/show_bug.cgi?id=34136, which has too much discussion on it to really be actionable at this point.

lib/CodeGen/SelectionDAG/SDNodeDbgValue.h
73–74

SDDbgValue creation is kind of an implementation detail of SelectionDAG. I think it might be better to collapse the frame index and virtual register constructor overloads and add a SDDbgValue::DbgValueKind parameter that SelectionDAG::getFrameIndexDbgValue and getVRegDbgValue fill in appropriately.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4974–4988

So, this should only be called in the case where the value of the variable is the address of some stack location, i.e. int x = 0; int *px = &x;, after optimization, we'll have this direct frame index dbg.value for px. I think that source fragment in a comment would help clarify why IsIndirect is false.

5199

Yikes, this was easier to read before rL314363. INT_MAX is better. =P

vsk updated this revision to Diff 156942.Jul 23 2018, 5:36 PM
vsk marked an inline comment as done.

Unify the SDDbgValue constructor for vregs and frame-indices; add a more descriptive comment in ::getDbgValue about why a FrameIndexDbgValue must be direct.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4974–4988

Isn't there one more case, where the value of the variable lives inside of some stack location?

To build on your example, I think the case you described looks like this:

dbg.value(i32* %px, !"int *px", !DIExpression())

And the one I'm pointing out looks like this:

dbg.value(i32* %px, !"int x", !DIExpression(DW_OP_deref))
shafik added inline comments.Jul 23 2018, 9:29 PM
include/llvm/CodeGen/SelectionDAG.h
1253–1254

Instead of using a bool for IsIndirect perhaps using an enum with two enumerator Direct and Indirect would be more readable and maintainable?

I make this comment since I noticed you were commenting your usage below, which indicated lack of clarity.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
700–708

Does using auto gain us much here?

lib/IR/DebugInfoMetadata.cpp
855

Are we expecting to only do this once and that is why we don't clear NewOps after the appendToVector below?

vsk updated this revision to Diff 157056.Jul 24 2018, 9:13 AM
vsk marked an inline comment as done.
vsk added inline comments.
include/llvm/CodeGen/SelectionDAG.h
1253–1254

I think commented use of a bool is reasonably clear. Refactoring the code to use enums would be fine as a follow-up change, but doesn't seem necessary for this patch.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
700–708

Yes, it's the convention in the backends to feign ignorance as to what BuildMI returns:

$ gg -E "auto [^=]+= BuildMI" | wc -l
      52

It's a bit distracting to see 'MachineInstrBuilder' here, c.f unordered_map iterators.

lib/IR/DebugInfoMetadata.cpp
855

Right. I'll try to make that clearer in the final comment in this function.

rnk accepted this revision.Jul 26 2018, 11:06 AM

lgtm

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4974–4988

Right, yes, which is the original motivating example for inserting dbg.values before calls.

This revision is now accepted and ready to land.Jul 26 2018, 11:06 AM
This revision was automatically updated to reflect the committed changes.