[PATCH] D58831: [DebugInfo] Ignore bitcasts when lowering stack arg dbg.values

[David Stenberg via Phabricator via llvm-commits]

dstenb marked an inline comment as done.
dstenb added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5309
     // Check if frame index is available.
-    if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(N.getNode()))
+    SDValue LCandidate = peekThroughBitcasts(N);
+    if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(LCandidate.getNode()))
----------------
bjope wrote:
> aprantl wrote:
> > 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?
> 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!
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?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58831/new/

https://reviews.llvm.org/D58831