[PATCH] D57702: [SelectionDAGBuilder] Add restrictions to EmitFuncArgumentDbgValue

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

>   plus it'd be good to hear Adrians view on the fragment scenario.

Do you have any specific questions in mind?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5011
+    // We also want to check that the parameter value V actually maps to the
+    // argument described by Variable. If we for example have IR like this
+    //
----------------
I don't understand why this is necessary?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5023
+    // is an input parameter. But they are not really related to each other.
+    // I'm not really sure how to check that, so instead we make sure that we do
+    // not describe the same argument twice since that seems to do the trick.
----------------
That is not generally possible. For example:
```
$ cat /tmp/s.c 
struct s { long long int i, j; };

int f(struct s s) {
  return s.i+s.j;
}

$ clang -g -S -emit-llvm /tmp/s.c -o -
; ModuleID = '/tmp/s.c'
source_filename = "/tmp/s.c"
target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.14.0"

%struct.s = type { i64, i64 }

; Function Attrs: noinline nounwind optnone ssp uwtable
define i32 @f(i64 %s.coerce0, i64 %s.coerce1) #0 !dbg !8 {
entry:

```

Here you'd have two dbg.values referring to the same DILocalVariable, but with different DIExpressions (with different DW_OP_LLVM_fragment). Since this case is so easy to reproduce, it would be good to at least handle it. The opposite (two DILocalVariables referring to the same function parameter) seems less interesting to me.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5025-5032
+    if (VariableIsFunctionInputArg) {
+      unsigned ArgNo = Arg->getArgNo();
+      if (ArgNo >= FuncInfo.DescribedArgs.size())
+        FuncInfo.DescribedArgs.resize(ArgNo + 1);
+      else if (!IsInPrologue && FuncInfo.DescribedArgs.test(ArgNo))
+        return false;
+      FuncInfo.DescribedArgs.set(ArgNo);
----------------
jmorse wrote:
> bjope wrote:
> > jmorse wrote:
> > > Will this work with Arguments that are referred to by multiple fragments? Perhaps it doesn't need to, however the two-i32s-packed-in-i64-argument in PR40188 sprung to mind as that produces two DBG_VALUEs based on one Argument.
> > At one point in time I tried using `ArgNo = Variable->getArg()`, looking at the C-level argument numbers, but then I had a test case that failed due to receiving that single struct variable in multiple registers (multiple fragments).
> > Now I use the argument numbers from the function on IR level. So at least that test case passed, since we get one fragment per input register. I guess it will be the same if the value is divided in multiple stack slots.
> > 
> > So I actually had problem with fragments in my first attempts to fix this, but I have no (real, non handwritten IR) test cases failing with the current solution.
> > Maybe there can be scenarios when we have multiple fragments mapping to one argument on IR level? (probably easy to do when doing a handwritten test case, but does it happen in reality)
> > 
> > So we might need to "complicate" this code even more in the future, or even find a totally different solution (the special case handling for function arguments seems a little bit messy IMHO). Maybe the frontend shouldn't use dbg.declare/dbg.value to describe input arguments, maybe we need dbg.argument or some other way to model this (if we really care about debug info for unused arguments, and if we really care about debug info inside the prologue)? 
> > 
> > For now, as far as I've seen (mainly when looking at C-programs), this patch seems to improve things. Avoiding faulty placement of DBG_VALUE nodes due to incorrectly treating them as describing function arguments. There is a small risk that it is too defensive in some situations. But instead of getting weird values (like with the faulty hoisting), being defensive in this case means that we would get <optimized out> in the debugger.
> > [...] a test case that failed due to receiving that single struct variable in multiple registers (multiple fragments)
> 
> Ah, my use of "fragments" was too vague, it was intended to mean:
> 
> > Maybe there can be scenarios when we have multiple fragments mapping to one argument on IR level?
> 
> Which I tried to produce an example for, but I hit some extra bugs along the way: [0]. In the example code there [0] the struct argument comes in as one i64, and SROA splits the dbg.declare into two dbg.values that refer to different variable fragments in their DIExpressions, but both ultimately refer to the same input Argument. If it weren't for [0] I believe (95%) they'd be salvaged back to two dbg.values operating on the input Argument.
> 
> > For now, as far as I've seen (mainly when looking at C-programs), this patch seems to improve things.
> 
> I agree, the argument-handling-weirdness has bugged me for a while, and this patch reduces the amount false debug information we produce. If multiple fragments of one Argument can be an issue we'll definitely hit it when solving [0].
> 
> [0] https://bugs.llvm.org/show_bug.cgi?id=40645
> Maybe there can be scenarios when we have multiple fragments mapping to one argument on IR level? (probably easy to do when doing a handwritten test case, but does it happen in reality)

The Swift compiler splats the elements of structs into individual function arguments and has a function signature optimization pass. If the optimizer were better at preserving debug info this combination could trigger what you are describing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57702