This is an archive of the discontinued LLVM Phabricator instance.

Debugging of optimized code: When locals have their address taken as part of a call use their stack slots as location expressions for debug info.
ClosedPublic

Authored by wolfgangp on Aug 8 2016, 3:24 PM.

Details

Summary

This patch proposes a small improvement to debug information for local variables whose address is taken and passed to a function. It is only relevant with optimization.

Example:

/******/
int data = 17;
int zero = 0;
int *ptr;

extern void foo(int i, int *p);

int main()
{

int val;
val = data;
foo(1, &val); // <== generates indirect+0 entry
foo(2, &data);
return zero;

}
/*******/

Compiled with -O2 -g -fno-omit-frame-pointer, the dwarfdump output for 'val' shows (Linux-X86):

DW_TAG_variable

DW_AT_location              <loclist with 2 entries follows>
[ 0]<lowpc=0x0000000e><highpc=0x00000015>DW_OP_reg0 DW_OP_piece 4
[ 1]<lowpc=0x00000015><highpc=0x0000001f>DW_OP_breg4+0
DW_AT_name                  "val"

The second location list entry for 'val' is an indirection of the register that gets the address of 'val', taken by the first call to 'foo()'.
That location list entry is only valid until the register is killed, which causes the range for the entry to be shorter than what would be feasible.

The patch proposes to detect FrameIndexSDNodes in SelectionDAG and generate FrameIndexDbgValues for them when we see a DW_op_deref expression attached to the incoming node. This will ultimately generate DBG_VALUE instructions with stack location operands, which will be valid longer than the current indirections. With the patch, dwarfdump shows:

DW_TAG_variable

DW_AT_location              <loclist with 2 entries follows>
[ 0]<lowpc=0x0000000e><highpc=0x00000015>DW_OP_reg0 DW_OP_piece 4
[ 1]<lowpc=0x00000015><highpc=0x0000003a>DW_OP_breg6-4
DW_AT_name                  "val"

Which makes 'val' visible in the debugger longer.

Note that when the frame pointer is eliminated, we don't get a longer range for the second entry in this example. This is due to the fact that subsequent call instructions are currently deemed to imp-def the stack pointer, ending the range for val's location prematurely. This is an orthogonal issue and is best addressed separately.

Diff Detail

Event Timeline

wolfgangp updated this revision to Diff 67231.Aug 8 2016, 3:24 PM
wolfgangp retitled this revision from to Debugging of optimized code: When locals have their address taken as part of a call use their stack slots as location expressions for debug info..
wolfgangp updated this object.
wolfgangp added reviewers: dblaikie, aprantl, echristo.
wolfgangp added a subscriber: llvm-commits.

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

aprantl edited edge metadata.EditedAug 9 2016, 10:04 AM

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

I haven't checked the code yet, but maybe you already know the answer: Does this also work if the variable has more than one value? If the FrameIndexDbgValue also gets lowered into DBG_VALUE we should be fine. If it ends up as a stack slot entry in the MMI table the following example should break:

int foo(bool c) {
  int i = 0;
  if (c)
    i = 1;
  else
    bar(&i)
  return i;
}

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

No, I don't have a counterexample, but this was conservatively directed at locals that have their address taken as part of a call, based on code in LowerDbgDeclare() in Local.cpp that explicitly generates a single DW_op_deref in that case. It may be possible to improve on it, but I haven't had time to look at any more complex examples yet.

I haven't checked the code yet, but maybe you already know the answer: Does this also work if the variable has more than one value? If the FrameIndexDbgValue also gets lowered into DBG_VALUE we should be fine. If it ends up as a stack slot entry in the MMI table the following example should break:

int foo(bool c) {
  int i = 0;
  if (c)
    i = 1;
  else
    bar(&i)
  return i;
}

The frameindex is preserved in the DBG_VALUEs, the actual stack slot doesn't get assigned until the PrologueEpilogueInserter/Frame Finalization pass, so I think from that point of view it's fine, but your example actually does not benefit from the change. It does what it used to do, i.e. generate an indirect reg+0 DBG_VALUE. I have to check why.

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

This is true if the stack slot is represented by a DBG_VALUE. If it is stored in the MMI table (together with the other dbg.declares) none of these passes even see it.

Thanks!

Why does this need to be restricted to "simple" derefs? I understand that it's easier to convince ourselves that a single DW_OP_deref is correct, but do you have a counterexample where checking whether the first opcode in the expression is a deref would not be correct?

No, I don't have a counterexample, but this was conservatively directed at locals that have their address taken as part of a call, based on code in LowerDbgDeclare() in Local.cpp that explicitly generates a single DW_op_deref in that case. It may be possible to improve on it, but I haven't had time to look at any more complex examples yet.

My intuition is that it shouldn't matter what other operations we perform on the value stored in the stack slot after dereferencing the pointer; in particular we should ignore any DW_OP_piece at the end of the expression.

Does this do the right thing if the stack offset is reused for another purpose via the lifetime intrinsics?

If a stack slot is reused for something else one of the later passes (LiveDebugVariables, LiveDebugValues or DbgValueHistoryCalculator) should detect this and generate the correct location info. That said, in my - admittedly simple - examples I haven't seen a case where stack slots are actually shared between different stack objects (user variables is what we're interested here), so it's possible I have missed something.

This is true if the stack slot is represented by a DBG_VALUE. If it is stored in the MMI table (together with the other dbg.declares) none of these passes even see it.

But doesn't lowerDbgDeclare() generate dbg.value intrinsics for all uses from the list of dbg.declares? That way they are exposed to the 3 passes.

Everything described by DBG_VALUE instructions is handled by LiveDebugValues etc.
Note, however that llvm::LowerDbgDeclare() currently does not remove all dbg.declare (arrays are skipped, for example).

My concern was that SelectionDAG would enter SDDbgValues describing a FrameIndex into the MMI sidetable, but I just confirmed that this isn't the case. It only calls MMI.setVariableDbgInfo() for dbg.declare instrinsics. So this is fine.

wolfgangp updated this revision to Diff 67606.Aug 10 2016, 2:36 PM
wolfgangp edited edge metadata.

Revised patch to account for DanglingDebugInfo nodes, which I had overlooked in the first attempt. Relaxed the check for a deref node to allow more general DI expressions with a top DW_op_deref node.

One more test case needed modification (array.ll).

aprantl accepted this revision.Aug 11 2016, 10:28 AM
aprantl edited edge metadata.

Found one bug, but otherwise this is looking good now, thanks!
LGTM (with all remaining review comments are addressed).

include/llvm/IR/DebugInfoMetadata.h
2159

Please add a doxygen comment to the function.
Should we rename this to startsWithDeref?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4751

I think we usually write this as "auto *FISDN = ..."

4758

After relaxing the condition, you'll want the tail of the dwarf expression here, not just an empty expression.

test/DebugInfo/AArch64/coalescing.ll
28

Could you please add a comment with the disassembled dwarf expression?

This revision is now accepted and ready to land.Aug 11 2016, 10:28 AM

Thanks for the review.

include/llvm/IR/DebugInfoMetadata.h
2159

Sure, sounds better.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4758

Oh, yes, thanks for catching that.

This revision was automatically updated to reflect the committed changes.