[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()))
----------------
dstenb wrote:
> dstenb wrote:
> > 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?
> I wrote https://bugs.llvm.org/show_bug.cgi?id=40973 for those issues now.
> 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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58831