Page MenuHomePhabricator

[DebugInfo] Ignore bitcasts when lowering stack arg dbg.values
ClosedPublic

Authored by dstenb on Mar 1 2019, 7:29 AM.

Details

Summary

Look past bitcasts when looking for parameter debug values that are
described by frame-index loads in EmitFuncArgumentDbgValue().

In the attached test case we would be left with an undef DBG_VALUE
for the parameter without this patch.

A similar fix was done for parameters passed in registers in D13005.

This fixes PR40777.

Diff Detail

Repository
rL LLVM

Event Timeline

dstenb created this revision.Mar 1 2019, 7:29 AM
jmorse added a comment.Mar 1 2019, 8:07 AM

LGTM, although I'll wait a bit for more opinions

aprantl accepted this revision.Mar 1 2019, 8:16 AM
aprantl added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5309 ↗(On Diff #188916)

LGTM; is this the most general place where to do this, or could we do this higher up in the function and catch more cases?

This revision is now accepted and ready to land.Mar 1 2019, 8:16 AM
bjope added a subscriber: bjope.Mar 1 2019, 8:39 AM
bjope added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5309 ↗(On Diff #188916)

I actually asked for a test case when the argument was in a register (hitting the uses of N above) when discussing this with David offline. Just to see if we were missing out on doing the same thing higher up in the function.
The answer from David was that there already is a test case for that scenario. We already peek-through-bitcast in getUnderlyingArgReg above, and there is a test case triggering that scenario.

One could argue that we should use getUnderlyingArgReg here as well. However, that one is also dealing with AssertSext etc. We have no test cases verifying correctness for those non-BITCAST-peek-through's done by getUnderlyingArgReg (maybe it is impossible to get trigger such situations here). So it feels more safe to only focus on BITCAST here right now.

So, LGTM!

dstenb marked an inline comment as done.Mar 5 2019, 7:41 AM
dstenb added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5309 ↗(On Diff #188916)

Sorry, when putting together this patch I was unable to trigger any {Assert[SE]ext, Truncate} cases for frame indices, but it seems that it's possible.

For example for the p4 parameter in the following test case (compiled using --target=arm -mbig-endian -O2 -g):

extern void bar(short);

void foo(int p0, int p1, int p2, int p3, short p4) {
  bar(p4);
}

for which we have:

t14: i16 = truncate t13                                                                                                                                                                                                                                                                     
  t13: i32 = AssertSext t11, ValueType:ch:i16                                                                                                                                                                                                                                               
    t11: i32,ch = load<(load 4 from %fixed-stack.4294967295)> t0, FrameIndex:i32<-1>, undef:i32                                                                                                                                                                                             
      t0: ch = EntryToken

As the target is big-endian, I think we need to add an offset of 2 bytes to the debug expression so that we point to the right part of the 4-byte stack slot.

Debug values for parameters that are smaller than the stack slot seems to be a bit broken for big-endian targets. If I remove the call to bar, so that p4 is left unused, we instead end up with the following node passed to EmitFuncArgumentDbgValue:

t11: i32,ch = load<(load 4 from %fixed-stack.4294967295)> t0, FrameIndex:i32<-1>, undef:i32                                                                                                                                                                                                 
  t0: ch = EntryToken

resulting in the following DBG_VALUE after isel:

DBG_VALUE %fixed-stack.0, 0, !"p4", !DIExpression(), debug-location !23; reduce.c:3:48 line no:3

ultimately leading to the following debug value in the output: @DEBUG_VALUE: foo:p4 <- [$sp+0]. I have not been able to verify this with a debugger yet, but unless I'm overlooking something I think the parameter will be printed using the wrong 2 bytes of the stack slot?

Would it be okay for me to deliver this look-past-bitcast patch as a first step (perhaps with a FIXME comment in the code), and then at least write a TR for the other issue?

dstenb marked an inline comment as done.Mar 6 2019, 12:46 AM
dstenb added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5309 ↗(On Diff #188916)

I wrote https://bugs.llvm.org/show_bug.cgi?id=40973 for those issues now.

dstenb marked an inline comment as done.Mar 13 2019, 8:59 AM
dstenb added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5309 ↗(On Diff #188916)

Would it be okay for me to deliver this look-past-bitcast patch as a first step (perhaps with a FIXME comment in the code), and then at least write a TR for the other issue?

I'll go ahead and deliver this patch before the end of the week unless there are any objections, as it should be a clear improvement, and the issue with the truncates will probably require some more invasive changes, and I don't know how to proceed with that as of now.

This revision was automatically updated to reflect the committed changes.