[DebugInfo][InstrRef] Don't produce over-sized DW_OP_deref_size expressions for very wide stack spills

[jmorse]

Summary

Addresses https://github.com/llvm/llvm-project/issues/64093 , where instruction referencing is producing an over-sized DW_OP_deref_size expression.

When we describe the locations of variables that have been spilt to the stack, there are occasions where we need to use DW_OP_deref_size, to indicate "this 32 bit variable is the lower order bits of the 64 bit value at this location". This has a meaningful difference on big endian systems, because the consumer can't necessarily know that the smaller value it's looking for is at a different position in a large spill slot.

It's not completely clear to me that instruction referencing is doing this correctly; it also probably isn't useful on a little endian system like x86. Fix the most obvious problem first: the DWARF spec says that the size of DW_OP_deref_size must not exceed the natural address size of the machine, so don't try to produce a DW_OP_deref_size if the operand would be bigger than the pointer size. Test comes from the github bug. While we're at it, refactor the logic into a utility function.

[Orlando]

Just a couple of inline nits/questions from me.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1163

Does a test for this part already exist? I assume so, since this patch is just narrowing the use cases rather than adding the functionally, but wanted to check to be sure.

1175

Can ValueSizeInBits ever be a value that doesn't divide cleanly by 8? e.g. 1 from an i1 type (I haven't looked at how getLocSizeInBits works).

If so then I think we might need to use divideCeil from MathExtras.h here. Otherwise, with the code as is, a 1 would result in DerefSizeInBytes of 0.

1181

left over dev comment?

1300

Same question about bit sizes and divisions here.

[aprantl]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1175

Would using a PointerSizeInBits instead help?

[dblaikie]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1177–1178

Could you clarify/rephrase the "output will identify a flawed location" - I don't understand what you mean by that & I'd have expected the failure here to be "absence of a location" rather than possibly incorrect locations ("flawed" I assume means "incorrect"?)

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_deref_size_too_big.mir

6

How would a plain DW_OP_deref work here - it'd still only be word-wide, right - so 64 bits, but the low 64 bit of a 128bit floating point value presumably isn't a valid floating point value, right?

[jmorse]

Sorry for the month delay, I was wandering up some mountains,

Coming back to this with fresh eyes, I missed something in the reproducer from GH ticket -- I thought it was spilling an entire vector register to the stack, however...

push    rbp
mov     rbp, rsp
sub     rsp, 16
movsd   qword ptr [rbp - 8], xmm0
call    c()@PLT
movsd   xmm0, qword ptr [rbp - 8]       # xmm0 = mem[0],zero
call    b(double)@PLT
add     rsp, 16
pop     rbp
ret

... it's actually performing a partial spill of the lower parts of xmm0, a fact that is encoded in the opcode (movsd not mov) but missed by InstrRefBasedLDV. So there's an extra factor of "where does that information go missing" to build into a different patch.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1163

Yup, it's llvm/test/DebugInfo/MIR/InstrRef/deref-spills-with-size.mir and tests pretty much every combination of sizes and derefs.

1175

I suppose switching to using bits instead of bytes will remove a category of weirdness that can occur, will implement that,

1177–1178

Improved wording would be "There's a risk this is incorrect on big endian systems", however I'm going to back off this for a moment, see main comment.

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_deref_size_too_big.mir

6

Hrrm. It's technically valid for x86 as the 128-bit-and-higher registers are all vectors, but baking that assumption in here guarantees it'll break something else. I'm going to back off from this for a moment (see main comment) and come back with a re-rationalised approach.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

bool operator!=(const MLocIterator &Other) const {
       return !(*this == Other);
     }
 
     void operator++() { Idx = LocIdx(Idx.asU64() + 1); }
 
     value_type operator*() { return value_type(Idx, ValueMap[LocIdx(Idx)]); }
   };
 
+  unsigned PointerSizeBytes = 0;
+
   MLocTracker(MachineFunction &MF, const TargetInstrInfo &TII,
               const TargetRegisterInfo &TRI, const TargetLowering &TLI);
 
   /// Produce location ID number for a Register. Provides some small amount of
   /// type safety.
   /// \param Reg The register we're looking up.
   unsigned getLocID(Register Reg) { return Reg.id(); }
 
public:
   iterator_range<MLocIterator> locations() {
     return llvm::make_range(begin(), end());
   }
 
   std::string LocIdxToName(LocIdx Idx) const;
 
   std::string IDAsString(const ValueIDNum &Num) const;
 
+  bool useDerefSize(const DebugVariable &Var, LocIdx MLoc,
+                    const DIExpression *Expr) const;
+
 #ifndef NDEBUG
   LLVM_DUMP_METHOD void dump();
 
   LLVM_DUMP_METHOD void dump_mloc_map();
 #endif
 
   /// Create a DBG_VALUE based on debug operands \p DbgOps. Qualify it with the
   /// information in \pProperties, for variable Var. Don't insert it anywhere,

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

MLocTracker::MLocTracker(MachineFunction &MF, const TargetInstrInfo &TII,
                          const TargetLowering &TLI)
     : MF(MF), TII(TII), TRI(TRI), TLI(TLI),
       LocIdxToIDNum(ValueIDNum::EmptyValue), LocIdxToLocID(0) {
   NumRegs = TRI.getNumRegs();
   reset();
   LocIDToLocIdx.resize(NumRegs, LocIdx::MakeIllegalLoc());
   assert(NumRegs < (1u << NUM_LOC_BITS)); // Detect bit packing failure
 
+  PointerSizeBytes = MF.getTarget().getPointerSize(0);
+
   // Always track SP. This avoids the implicit clobbering caused by regmasks
   // from affectings its values. (LiveDebugValues disbelieves calls and
   // regmasks that claim to clobber SP).
   Register SP = TLI.getStackPointerRegisterToSaveRestore();
   if (SP) {
     unsigned ID = getLocID(SP);
     (void)lookupOrTrackRegister(ID);
 

 LLVM_DUMP_METHOD void MLocTracker::dump_mloc_map() {
   for (auto Location : locations()) {
     std::string foo = LocIdxToName(Location.Idx);
     dbgs() << "Idx " << Location.Idx.asU64() << " " << foo << "\n";
   }
 }
 #endif
 
+bool MLocTracker::useDerefSize(const DebugVariable &Var, LocIdx MLoc,
+                               const DIExpression *Expr) const {
+  // We need to use deref_size whenever there's a mismatch between the
+  // size of value and the size of variable portion being read, on big endian
+  // systems. Consider:
+  //
+  //    store [r1], r2.64bits
+  //    DBG_VALUE [r1], ... ; 32-bit-variable
+  //
+  // If we widen the store of a 32 bit variable into a 64 bit stack spill, on
+  // little-endian systems the 32 lower bits always end up in the same place.
+  // However, on big-endian systems this is not true, so we need to describe
+  // it as "the lower 32 bits that you get after a 64 bit load", by using
+  // DW_OP_deref_size.
+
+  unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
+  unsigned DerefSizeInBytes = ValueSizeInBits / 8;
+  if (DerefSizeInBytes > PointerSizeBytes) {
+    // This would be illegal DWARF. Perhaps the output will identify a
+    // flawed location, but it'll at least be legal output.
+    return false;
+  } else if (auto Fragment = Var.getFragment()) {
+    // XXX why the non-complex bit.
+    unsigned VariableSizeInBits = Fragment->SizeInBits;
+    if (VariableSizeInBits != ValueSizeInBits || Expr->isComplex())
+      return true;
+  } else if (auto Size = Var.getVariable()->getSizeInBits()) {
+    if (*Size != ValueSizeInBits)
+      return true;
+  }
+
+  return false;
+}
+
 MachineInstrBuilder
 MLocTracker::emitLoc(const SmallVectorImpl<ResolvedDbgOp> &DbgOps,
                      const DebugVariable &Var,
                      const DbgValueProperties &Properties) {
   DebugLoc DL = DILocation::get(Var.getVariable()->getContext(), 0, 0,
                                 Var.getVariable()->getScope(),
                                 const_cast<DILocation *>(Var.getInlinedAt()));
 
if (LocID >= NumRegs) {
         unsigned Base = Spill.SpillBase;
 
         // There are several ways we can dereference things, and several inputs
         // to consider:
         // * NRVO variables will appear with IsIndirect set, but should have
         //   nothing else in their DIExpressions,
         // * Variables with DW_OP_stack_value in their expr already need an
         //   explicit dereference of the stack location,
-        // * Values that don't match the variable size need DW_OP_deref_size,
+        // * Values that don't match the variable size potentially need
+        //   DW_OP_deref_size to describe them,
         // * Everything else can just become a simple location expression.
-
-        // We need to use deref_size whenever there's a mismatch between the
-        // size of value and the size of variable portion being read.
-        // Additionally, we should use it whenever dealing with stack_value
-        // fragments, to avoid the consumer having to determine the deref size
-        // from DW_OP_piece.
-        bool UseDerefSize = false;
-        unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
-        unsigned DerefSizeInBytes = ValueSizeInBits / 8;
-        if (auto Fragment = Var.getFragment()) {
-          unsigned VariableSizeInBits = Fragment->SizeInBits;
-          if (VariableSizeInBits != ValueSizeInBits || Expr->isComplex())
-            UseDerefSize = true;
-        } else if (auto Size = Var.getVariable()->getSizeInBits()) {
-          if (*Size != ValueSizeInBits) {
-            UseDerefSize = true;
-          }
-        }
-
         SmallVector<uint64_t, 5> OffsetOps;
         TRI.getOffsetOpcodes(Spill.SpillOffset, OffsetOps);
         bool StackValue = false;
+        bool UseDerefSize = useDerefSize(Var, MLoc, Expr);
 
         if (Properties.Indirect) {
           // This is something like an NRVO variable, where the pointer has been
           // spilt to the stack. It should end up being a memory location, with
           // the pointer to the variable loaded off the stack with a deref:
           assert(!Expr->isImplicit());
           OffsetOps.push_back(dwarf::DW_OP_deref);
         } else if (UseDerefSize && Expr->isSingleLocationExpression()) {
           // TODO: Figure out how to handle deref size issues for variadic
           // values.
           // We're loading a value off the stack that's not the same size as the
           // variable. Add / subtract stack offset, explicitly deref with a
           // size, and add DW_OP_stack_value if not already present.
+          unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
+          unsigned DerefSizeInBytes = ValueSizeInBits / 8;
           OffsetOps.push_back(dwarf::DW_OP_deref_size);
           OffsetOps.push_back(DerefSizeInBytes);
           StackValue = true;
         } else if (Expr->isComplex() || Properties.IsVariadic) {
           // A variable with no size ambiguity, but with extra elements in it's
           // expression. Manually dereference the stack location.
           OffsetOps.push_back(dwarf::DW_OP_deref);
         } else {

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_deref_size_too_big.mir

+# RUN: llc %s -o - --run-pass=livedebugvalues | FileCheck %s
+#
+# Test that when a small variable ("e", a double) in a large register (xmm0,
+# which is 128 bits wide) is spilt, we don't produce a DW_OP_deref_size that
+# exceeds the size of an address (64 bits). We should produce a plain
+# DW_OP_deref instead, as that's a legal output.
+#
+# See https://github.com/llvm/llvm-project/issues/64093
+#
+# CHECK: DBG_VALUE $rsp, 0,
+#
+# (That's an indirect location for the variable, at the stack pointer, where
+#  it's just been spilt).
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  define dso_local void @e(double noundef %e) local_unnamed_addr #0 !dbg !10 {
+  entry:
+    call void @llvm.dbg.value(metadata double %e, metadata !15, metadata !DIExpression()), !dbg !16
+    tail call void (...) @c() #3, !dbg !17
+    tail call void @b(double noundef %e) #3, !dbg !18
+    ret void, !dbg !19
+  }
+  
+  declare !dbg !20 void @c(...) local_unnamed_addr #1
+  
+  declare !dbg !23 void @b(double noundef) local_unnamed_addr #1
+  
+  ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
+  !llvm.ident = !{!9}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test3.c", directory: "/tmp")
+  !2 = !{i32 7, !"Dwarf Version", i32 5}
+  !3 = !{i32 2, !"Debug Info Version", i32 3}
+  !4 = !{i32 1, !"wchar_size", i32 4}
+  !5 = !{i32 8, !"PIC Level", i32 2}
+  !6 = !{i32 7, !"PIE Level", i32 2}
+  !7 = !{i32 7, !"uwtable", i32 2}
+  !8 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+  !9 = !{!"clang"}
+  !10 = distinct !DISubprogram(name: "e", scope: !1, file: !1, line: 3, type: !11, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !14)
+  !11 = !DISubroutineType(types: !12)
+  !12 = !{null, !13}
+  !13 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+  !14 = !{!15}
+  !15 = !DILocalVariable(name: "e", arg: 1, scope: !10, file: !1, line: 3, type: !13)
+  !16 = !DILocation(line: 0, scope: !10)
+  !17 = !DILocation(line: 4, column: 3, scope: !10)
+  !18 = !DILocation(line: 5, column: 3, scope: !10)
+  !19 = !DILocation(line: 6, column: 1, scope: !10)
+  !20 = !DISubprogram(name: "c", scope: !1, file: !1, line: 2, type: !21, spFlags: DISPFlagOptimized)
+  !21 = !DISubroutineType(types: !22)
+  !22 = !{null}
+  !23 = !DISubprogram(name: "b", scope: !1, file: !1, line: 1, type: !11, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+
+...
+---
+name:            e
+alignment:       16
+tracksRegLiveness: true
+debugInstrRef:   true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$xmm0' }
+frameInfo:
+  stackSize:       8
+  offsetAdjustment: -8
+  maxAlignment:    8
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+  hasTailCall:     true
+stack:
+  - { id: 0, type: spill-slot, offset: -16, size: 8, alignment: 8 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $xmm0
+  
+    DBG_VALUE $xmm0, $noreg, !15, !DIExpression(), debug-location !16
+    frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 16
+    MOVSDmr $rsp, 1, $noreg, 0, $noreg, $xmm0 :: (store (s64) into %stack.0)
+    dead $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $al, debug-location !17
+    CALL64pcrel32 target-flags(x86-plt) @c, csr_64, implicit $rsp, implicit $ssp, implicit killed $al, implicit-def $rsp, implicit-def $ssp, debug-location !17
+    $xmm0 = MOVSDrm_alt $rsp, 1, $noreg, 0, $noreg :: (load (s64) from %stack.0)
+    $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !18
+    frame-destroy CFI_INSTRUCTION def_cfa_offset 8, debug-location !18
+    TAILJMPd64 target-flags(x86-plt) @b, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit $xmm0, debug-location !18
+
+...