[DebugInfo] Correctly place DW_OP_derefs for arguments passed on stack

[jmorse]

Summary

Following on from D68945, @uabelho pointed out a scenario where there's some confusion about where a DW_OP_deref should be added for arguments passed on the stack. A Value/Argument that gets placed in memory requires the stack slot it lands in to be dereferenced, and that deref would go at the start of the expression so that it operates on the slot. If the argument is the operand to a dbg.declare, then it might require a deref at the end of the expression as well, to make it a memory location.

In SelectionDAG::EmitFuncArgumentDbgValue that's what I've implemented, best illustrated by the baz function in the attached test:

define i8 @baz(i32 *%blah) !dbg !40 {
entry:
  call void @llvm.dbg.declare(metadata i32* %blah, metadata !43, metadata !DIExpression(DW_OP_plus_uconst, 4)), !dbg !41
  ret i8 0, !dbg !41
}

%blah is an incoming pointer, but it arrives in a stack slot, which must be dereferenced first. And because it is used by a dbg.declare, we append a deref to force it to be a memory location. With this patch, we get the DBG_VALUE:

DBG_VALUE %fixed-stack.0, $noreg, !123, !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4, DW_OP_deref)

i.e., "load the stack slot, fiddle with the pointer, then be a memory location". I've added llvm-dwarfdump checks to ensure this comes out the far end in the correct format (I'm 97% the encoding is correct there).

I've deleted the "IsIndirect" var in EmitFuncArgumentDbgValue, as it wasn't doing anything useful IMO.

[uabelho]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5558 ↗

(On Diff #225193)

Nice, I think this is heading the right direction!
However, if we look at PEI::replaceFrameIndices which handles the indirect stuff, it uses DW_OP_deref_size rather than DW_OP_deref:

if (MI.isIndirectDebugValue() && DIExpr->isImplicit()) {
  SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Size};
  bool WithStackValue = true;
  DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, WithStackValue);
  // Make the DBG_VALUE direct.
  MI.getOperand(1).ChangeToRegister(0, false);
}

I think we need to take the size into consideration here in SelectionDAGBuilder too.

My out-of-tree big-endian target example now goes wrong since we read a value that is placed adjacent to the wanted value on the stack. I think using DW_OP_deref_size could perhaps solve that.

[jmorse]

Cool, here's a version with deref_size; this could be refactored with the PrologEpilog site too, but it's not obvious which headers such a helper should live in.

The PrologEpilog site only uses deref_size if it's an implicit location, I've done the same here.

[probinson]

Just to clarify things in my own mind: dbg.declare expressions describe a location, while dbg.value expressions describe a value? And that's why you're wanting to add a trailing deref?
Then when we emit the actual DWARF, the trailing deref is omitted so the expression again describes a location.

[jmorse]

In D69028#1711085, @probinson wrote:

Just to clarify things in my own mind: dbg.declare expressions describe a location, while dbg.value expressions describe a value? And that's why you're wanting to add a trailing deref?

Yup, that's correct. I repeatedly screw up the correct use of the term "location", so to be super explicit, dbg.declare always describes a _memory_ location.

Then when we emit the actual DWARF, the trailing deref is omitted so the expression again describes a location.

Indeed.

As suggested in https://reviews.llvm.org/D68945#1710656 , I'd enjoy a future where the DwarfExpression code didn't have to guess what kind of location we were dealing with (no "unknown" state).

[uabelho]

In D69028#1711048, @jmorse wrote:

Cool, here's a version with deref_size;

Great, now that failing testcase passes! Thanks!

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

[uabelho]

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

That was probably false alarm, I think it's a problem in our testcase rather than in the patch.

[vsk]

This looks great.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5564 ↗

(On Diff #225220)

Out of curiosity, why can't the sized deref be used for stack values?

[jmorse]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5564 ↗

(On Diff #225220)

Alas, I've no idea -- this is just closely matching the existing code in PrologEpilog. Most likely answer IMO is that it would be legal, but there'd be large amounts of test changes for very little gain.

[vsk]

Thanks!

[uabelho]

In D69028#1712384, @uabelho wrote:

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

That was probably false alarm, I think it's a problem in our testcase rather than in the patch.

To be clear: It was false alarm. So I'm not aware of any problems with this patch.

Thanks!

[bjope]

(Reverse ping!) Any reason why this hasn't landed yet?

[jmorse]

Ah blast, I mentally filed this under "things to push upwards when there's spare time" as opposed to "a regression that needs fixing". Sorry for the delay, I'll give it a kick now.

[bjope]

In D69028#1727512, @jmorse wrote:

Ah blast, I mentally filed this under "things to push upwards when there's spare time" as opposed to "a regression that needs fixing". Sorry for the delay, I'll give it a kick now.

I think we considered adding this workaround downstream but ended up reverting the patch that broke our test case instead (or rather, we had already reverted it so this has not been highest prio for us).
Anyway, we are not really stuck due to this downstream, I just realized that it is a pity that we haven't moved forward with using (and testing) the new way of handling "IsIndirect" yet.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (VariableIsFunctionInputArg) {
         return false;
       FuncInfo.DescribedArgs.set(ArgNo);
     }
   }
 
   MachineFunction &MF = DAG.getMachineFunction();
   const TargetInstrInfo *TII = DAG.getSubtarget().getInstrInfo();
 
-  bool IsIndirect = false;
   Optional<MachineOperand> Op;
   // Some arguments' frame index is recorded during argument lowering.
   int FI = FuncInfo.getArgumentFrameIndex(Arg);
   if (FI != std::numeric_limits<int>::max())
     Op = MachineOperand::CreateFI(FI);
 
   SmallVector<std::pair<unsigned, unsigned>, 8> ArgRegsAndSizes;
   if (!Op && N.getNode()) {
     getUnderlyingArgRegs(ArgRegsAndSizes, N);
     Register Reg;
     if (ArgRegsAndSizes.size() == 1)
       Reg = ArgRegsAndSizes.front().first;
 
     if (Reg && Reg.isVirtual()) {
       MachineRegisterInfo &RegInfo = MF.getRegInfo();
       Register PR = RegInfo.getLiveInPhysReg(Reg);
       if (PR)
         Reg = PR;
     }
     if (Reg) {
       Op = MachineOperand::CreateReg(Reg, false);
-      IsIndirect = IsDbgDeclare;
     }
   }
 
   if (!Op && N.getNode()) {
     // Check if frame index is available.
     SDValue LCandidate = peekThroughBitcasts(N);
     if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(LCandidate.getNode()))
       if (FrameIndexSDNode *FINode =
if (VMI != FuncInfo.ValueMap.end()) {
       RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), VMI->second,
                        V->getType(), getABIRegCopyCC(V));
       if (RFV.occupiesMultipleRegs()) {
         splitMultiRegDbgValue(RFV.getRegsAndSizes());
         return true;
       }
 
       Op = MachineOperand::CreateReg(VMI->second, false);
-      IsIndirect = IsDbgDeclare;
     } else if (ArgRegsAndSizes.size() > 1) {
       // This was split due to the calling convention, and no virtual register
       // mapping exists for the value.
       splitMultiRegDbgValue(ArgRegsAndSizes);
       return true;
     }
   }
 
   if (!Op)
     return false;
 
   assert(Variable->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
-  IsIndirect = (Op->isReg()) ? IsIndirect : true;
-  if (IsIndirect)
+
+  // If the argument arrives in a stack slot, then what the IR thought was a
+  // normal Value is actually in memory, and we must add a deref to load it.
+  if (Op->isFI()) {
+    int FI = Op->getIndex();
+    unsigned Size = DAG.getMachineFunction().getFrameInfo().getObjectSize(FI);
+    if (Expr->isImplicit()) {
+      SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Size};
+      Expr = DIExpression::prependOpcodes(Expr, Ops);
+    } else {
+      Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
+    }
+  }
+
+  // If this location was specified with a dbg.declare, then it and its
+  // expression calculate the address of the variable. Append a deref to
+  // force it to be a memory location.
+  if (IsDbgDeclare)
     Expr = DIExpression::append(Expr, {dwarf::DW_OP_deref});
+
   FuncInfo.ArgDbgValues.push_back(
       BuildMI(MF, DL, TII->get(TargetOpcode::DBG_VALUE), false,
               *Op, Variable, Expr));
 
   return true;
 }
 
 /// Return the appropriate SDDbgValue based on N.

llvm/test/DebugInfo/X86/stack-arg-deref.ll

+; RUN: llc -stop-before=finalize-isel -o - %s -mtriple=i386-- | FileCheck %s --check-prefix=MIR
+; RUN: llc -o - %s -mtriple=i386-- --filetype=obj | llvm-dwarfdump - | FileCheck %s --check-prefix=DWARF --implicit-check-not=DW_TAG_subprogram
+; REQUIRES: object-emission
+;
+; Test that, when arguments are passed on the stack (such as i386),
+; variable location dereferences occur in the right place. When referring to
+; argument stack slots a deref must be used to load the slot first.
+
+; MIR: ![[FOOVAR:[0-9]+]] = !DILocalVariable(name: "foovar"
+; MIR: ![[BARVAR:[0-9]+]] = !DILocalVariable(name: "barvar"
+; MIR: ![[BAZVAR:[0-9]+]] = !DILocalVariable(name: "bazvar"
+
+; Plain i32 on the stack.
+; MIR-LABEL: name: foo
+; MIR:       DBG_VALUE %fixed-stack.0, $noreg, ![[FOOVAR]],
+; MIR-SAME:  !DIExpression(DW_OP_deref)
+; DWARF: DW_TAG_subprogram
+; DWARF-LABEL: DW_AT_name ("cheese")
+; DWARF:       DW_TAG_variable
+; DWARF-NEXT:  DW_AT_location (DW_OP_fbreg +4)
+; DWARF-NEXT:  DW_AT_name ("foovar")
+define i8 @foo(i32 %blah) !dbg !20 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %blah, metadata !23, metadata !DIExpression()), !dbg !21
+  ret i8 0, !dbg !21
+}
+
+; Pointer on the stack that we fiddle with.
+; MIR-LABEL: name: bar
+; MIR:       DBG_VALUE %fixed-stack.0, $noreg, ![[BARVAR]],
+; MIR-SAME:  !DIExpression(DW_OP_deref_size, 4, DW_OP_plus_uconst, 4, DW_OP_stack_value)
+; DWARF: DW_TAG_subprogram
+; DWARF-LABEL: DW_AT_name ("nope")
+; DWARF:       DW_TAG_variable
+; DWARF-NEXT:  DW_AT_location (DW_OP_fbreg +4, DW_OP_deref_size 0x4, DW_OP_plus_uconst 0x4, DW_OP_stack_value)
+; DWARF-NEXT:  DW_AT_name ("barvar")
+define i8 @bar(i32 *%blah) !dbg !30 {
+entry:
+  call void @llvm.dbg.value(metadata i32* %blah, metadata !33, metadata !DIExpression(DW_OP_plus_uconst, 4, DW_OP_stack_value)), !dbg !31
+  ret i8 0, !dbg !31
+}
+
+; Pointer that we use as a dbg.declare variable location, after fiddling with
+; the pointer value.
+; MIR-LABEL: name: baz
+; MIR:       DBG_VALUE %fixed-stack.0, $noreg, ![[BAZVAR]],
+; MIR-SAME:  !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4, DW_OP_deref)
+; DWARF: DW_TAG_subprogram
+; DWARF-LABEL: DW_AT_name ("brains")
+; DWARF:       DW_TAG_variable
+; DWARF-NEXT:  DW_AT_location (DW_OP_fbreg +4, DW_OP_deref, DW_OP_plus_uconst 0x4)
+; DWARF-NEXT:  DW_AT_name ("bazvar")
+define i8 @baz(i32 *%blah) !dbg !40 {
+entry:
+  call void @llvm.dbg.declare(metadata i32* %blah, metadata !43, metadata !DIExpression(DW_OP_plus_uconst, 4)), !dbg !41
+  ret i8 0, !dbg !41
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "asdf", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "nil", directory: "/")
+!2 = !{}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = !DISubroutineType(types: !2)
+!8 = !DIBasicType(name: "i32", size: 32, encoding: DW_ATE_signed)
+
+!20 = distinct !DISubprogram(name: "cheese", linkageName: "cheese", scope: null, file: !1, line: 12, type: !7, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !0, retainedNodes: !22)
+!21 = !DILocation(line: 1, column: 1, scope: !20)
+!22 = !{!23}
+!23 = !DILocalVariable(name: "foovar", scope: !20, file: !1, line: 14, type: !8)
+
+!30 = distinct !DISubprogram(name: "nope", linkageName: "nope", scope: null, file: !1, line: 12, type: !7, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !0, retainedNodes: !32)
+!31 = !DILocation(line: 1, column: 1, scope: !30)
+!32 = !{!33}
+!33 = !DILocalVariable(name: "barvar", scope: !30, file: !1, line: 14, type: !8)
+
+!40 = distinct !DISubprogram(name: "brains", linkageName: "brains", scope: null, file: !1, line: 12, type: !7, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !0, retainedNodes: !42)
+!41 = !DILocation(line: 1, column: 1, scope: !40)
+!42 = !{!43}
+!43 = !DILocalVariable(name: "bazvar", scope: !40, file: !1, line: 14, type: !8)