[DebugInfo][CSInfo] Avoid using clobbered registers as call-site argument locations

[jmorse]

Summary

As reported in https://github.com/llvm/llvm-project/issues/57444 , when we build call-site information and find a copy from a non-volatile to a parameter register, the non-volatile shouldn't be an argument location if that register is clobbered between the copy and the call. We need to perform that check for clobbering, or incorrect values could appear in the debugger as entry values.

This patch adds a collection of register units defined as we walk back from the call site, and prevents the acceptance of a call-site parameter location if it's going to be clobbered. It's still possible for a subsequent copy to that location to be accepted, if won't be clobbered. The added test checks this using the reproducer from the bug report.

In terms of call-site coverage, I've built stage2 RelWithDebInfo clang with and without this patch, based on rG2b1d45b227bd, and there are roughly 5% fewer DW_AT_call_value attributes produced once this patch is applied, which I think we can reasonably conclude were incorrect locations.

[djtodoro]

Looks reasonable to me.

In terms of call-site coverage, I've built stage2 RelWithDebInfo clang with and without this patch, based on rG2b1d45b227bd, and there are roughly 5% fewer DW_AT_call_value attributes produced once this patch is applied, which I think we can reasonably conclude were incorrect locations.

The number refers to x86 target, right?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

707

nit: Can we make a type alias for the small set?

[jmorse]

Use a type alias instead of SmallSet directly,

@djtodoro wrote:

The number refers to x86 target, right?

That's correct, yes.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

struct FwdRegParamInfo {
 
   /// Debug expression that has been built up when walking through the
   /// instruction chain that produces the parameter's value.
   const DIExpression *Expr;
 };
 
 /// Register worklist for finding call site values.
 using FwdRegWorklist = MapVector<unsigned, SmallVector<FwdRegParamInfo, 2>>;
+/// Container for the set of registers known to be clobbered on the path to a
+/// call site.
+using ClobberedRegSet = SmallSet<Register, 16>;
 
 /// Append the expression \p Addition to \p Original and return the result.
 static const DIExpression *combineDIExpressions(const DIExpression *Original,
                                                 const DIExpression *Addition) {
   std::vector<uint64_t> Elts = Addition->getElements().vec();
   // Avoid multiple DW_OP_stack_values.
   if (Original->isImplicit() && Addition->isImplicit())
     erase_value(Elts, dwarf::DW_OP_stack_value);
for (auto Param : ParamsToAdd) {
     const DIExpression *CombinedExpr = combineDIExpressions(Expr, Param.Expr);
     ParamsForFwdReg.push_back({Param.ParamReg, CombinedExpr});
   }
 }
 
 /// Interpret values loaded into registers by \p CurMI.
 static void interpretValues(const MachineInstr *CurMI,
                             FwdRegWorklist &ForwardedRegWorklist,
-                            ParamSet &Params) {
+                            ParamSet &Params,
+                            ClobberedRegSet &ClobberedRegUnits) {
 
   const MachineFunction *MF = CurMI->getMF();
   const DIExpression *EmptyExpr =
       DIExpression::get(MF->getFunction().getContext(), {});
   const auto &TRI = *MF->getSubtarget().getRegisterInfo();
   const auto &TII = *MF->getSubtarget().getInstrInfo();
   const auto &TLI = *MF->getSubtarget().getTargetLowering();
 
static void interpretValues(const MachineInstr *CurMI,
   // In order to not have to distinguish between those cases when finalizing
   // entry values, we simply postpone adding new parameter registers to the
   // worklist, by first keeping them in this temporary container until the
   // instruction has been handled.
   FwdRegWorklist TmpWorklistItems;
 
   // If the MI is an instruction defining one or more parameters' forwarding
   // registers, add those defines.
+  ClobberedRegSet NewClobberedRegUnits;
   auto getForwardingRegsDefinedByMI = [&](const MachineInstr &MI,
                                           SmallSetVector<unsigned, 4> &Defs) {
     if (MI.isDebugInstr())
       return;
 
     for (const MachineOperand &MO : MI.operands()) {
       if (MO.isReg() && MO.isDef() && MO.getReg().isPhysical()) {
         for (auto &FwdReg : ForwardedRegWorklist)
           if (TRI.regsOverlap(FwdReg.first, MO.getReg()))
             Defs.insert(FwdReg.first);
+        for (MCRegUnitIterator Units(MO.getReg(), &TRI); Units.isValid(); ++Units)
+          NewClobberedRegUnits.insert(*Units);
       }
     }
   };
 
   // Set of worklist registers that are defined by this instruction.
   SmallSetVector<unsigned, 4> FwdRegDefs;
 
   getForwardingRegsDefinedByMI(*CurMI, FwdRegDefs);
-  if (FwdRegDefs.empty())
+  if (FwdRegDefs.empty()) {
+    // Any definitions by this instruction will clobber earlier reg movements.
+    ClobberedRegUnits.insert(NewClobberedRegUnits.begin(),
+                             NewClobberedRegUnits.end());
     return;
+  }
+
+  // It's possible that we find a copy from a non-volatile register to the param
+  // register, which is clobbered in the meantime. Test for clobbered reg unit
+  // overlaps before completing.
+  auto IsRegClobberedInMeantime = [&](Register Reg) -> bool {
+    for (auto &RegUnit : ClobberedRegUnits)
+      if (TRI.hasRegUnit(Reg, RegUnit))
+        return true;
+    return false;
+  };
 
   for (auto ParamFwdReg : FwdRegDefs) {
     if (auto ParamValue = TII.describeLoadedValue(*CurMI, ParamFwdReg)) {
       if (ParamValue->first.isImm()) {
         int64_t Val = ParamValue->first.getImm();
         finishCallSiteParams(Val, ParamValue->second,
                              ForwardedRegWorklist[ParamFwdReg], Params);
       } else if (ParamValue->first.isReg()) {
         Register RegLoc = ParamValue->first.getReg();
         Register SP = TLI.getStackPointerRegisterToSaveRestore();
         Register FP = TRI.getFrameRegister(*MF);
         bool IsSPorFP = (RegLoc == SP) || (RegLoc == FP);
-        if (TRI.isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP) {
+        if (!IsRegClobberedInMeantime(RegLoc) &&
+            (TRI.isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP)) {
           MachineLocation MLoc(RegLoc, /*Indirect=*/IsSPorFP);
           finishCallSiteParams(MLoc, ParamValue->second,
                                ForwardedRegWorklist[ParamFwdReg], Params);
         } else {
           // ParamFwdReg was described by the non-callee saved register
           // RegLoc. Mark that the call site values for the parameters are
           // dependent on that register instead of ParamFwdReg. Since RegLoc
           // may be a register that will be handled in this iteration, we
           // postpone adding the items to the worklist, and instead keep them
           // in a temporary container.
           addToFwdRegWorklist(TmpWorklistItems, RegLoc, ParamValue->second,
                               ForwardedRegWorklist[ParamFwdReg]);
         }
       }
     }
   }
 
   // Remove all registers that this instruction defines from the worklist.
   for (auto ParamFwdReg : FwdRegDefs)
     ForwardedRegWorklist.erase(ParamFwdReg);
 
+  // Any definitions by this instruction will clobber earlier reg movements.
+  ClobberedRegUnits.insert(NewClobberedRegUnits.begin(),
+                           NewClobberedRegUnits.end());
+
   // Now that we are done handling this instruction, add items from the
   // temporary worklist to the real one.
   for (auto &New : TmpWorklistItems)
     addToFwdRegWorklist(ForwardedRegWorklist, New.first, EmptyExpr, New.second);
   TmpWorklistItems.clear();
 }
 
 static bool interpretNextInstr(const MachineInstr *CurMI,
                                FwdRegWorklist &ForwardedRegWorklist,
-                               ParamSet &Params) {
+                               ParamSet &Params,
+                               ClobberedRegSet &ClobberedRegUnits) {
   // Skip bundle headers.
   if (CurMI->isBundle())
     return true;
 
   // If the next instruction is a call we can not interpret parameter's
   // forwarding registers or we finished the interpretation of all
   // parameters.
   if (CurMI->isCall())
     return false;
 
   if (ForwardedRegWorklist.empty())
     return false;
 
   // Avoid NOP description.
   if (CurMI->getNumOperands() == 0)
     return true;
 
-  interpretValues(CurMI, ForwardedRegWorklist, Params);
+  interpretValues(CurMI, ForwardedRegWorklist, Params, ClobberedRegUnits);
 
   return true;
 }
 
 /// Try to interpret values loaded into registers that forward parameters
 /// for \p CallMI. Store parameters with interpreted value into \p Params.
 static void collectCallSiteParameters(const MachineInstr *CallMI,
                                       ParamSet &Params) {
static void collectCallSiteParameters(const MachineInstr *CallMI,
   // list, for which we do not describe a loaded value by
   // the describeLoadedValue(), we try to generate an entry value expression
   // for their call site value description, if the call is within the entry MBB.
   // TODO: Handle situations when call site parameter value can be described
   // as the entry value within basic blocks other than the first one.
   bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
 
   // Search for a loading value in forwarding registers inside call delay slot.
+  ClobberedRegSet ClobberedRegUnits;
   if (CallMI->hasDelaySlot()) {
     auto Suc = std::next(CallMI->getIterator());
     // Only one-instruction delay slot is supported.
     auto BundleEnd = llvm::getBundleEnd(CallMI->getIterator());
     (void)BundleEnd;
     assert(std::next(Suc) == BundleEnd &&
            "More than one instruction in call delay slot");
     // Try to interpret value loaded by instruction.
-    if (!interpretNextInstr(&*Suc, ForwardedRegWorklist, Params))
+    if (!interpretNextInstr(&*Suc, ForwardedRegWorklist, Params, ClobberedRegUnits))
       return;
   }
 
   // Search for a loading value in forwarding registers.
   for (; I != MBB->rend(); ++I) {
     // Try to interpret values loaded by instruction.
-    if (!interpretNextInstr(&*I, ForwardedRegWorklist, Params))
+    if (!interpretNextInstr(&*I, ForwardedRegWorklist, Params, ClobberedRegUnits))
       return;
   }
 
   // Emit the call site parameter's value as an entry value.
   if (ShouldTryEmitEntryVals) {
     // Create an expression where the register's entry value is used.
     DIExpression *EntryExpr = DIExpression::get(
         MF->getFunction().getContext(), {dwarf::DW_OP_LLVM_entry_value, 1});

llvm/test/DebugInfo/MIR/X86/dbgcall-site-clobbered-regs.mir

+# RUN: llc %s -o - -filetype=obj -start-after=livedebugvalues \
+# RUN:      -emit-call-site-info | \
+# RUN:  llvm-dwarfdump - | FileCheck %s --implicit-check-not=call_value
+#
+## Test that call-site information is produced for this input, but use the
+## implicit not above to check that no call value entries are produced for
+## the tail call. They're clobbered by the restoration of r14 and rbx.
+##
+## Addresses https://github.com/llvm/llvm-project/issues/57444
+#
+# CHECK:       DW_TAG_call_site
+# CHECK-NEXT:    DW_AT_call_origin
+# CHECK-NEXT:    DW_AT_call_return_pc
+#
+# CHECK:      DW_TAG_call_site
+# CHECK-NEXT:    DW_AT_call_origin
+# CHECK-NEXT:    DW_AT_call_tail_call  (true)
+# CHECK-NEXT:    DW_AT_call_pc
+
+--- |
+  ; ModuleID = 'out.ll'
+  source_filename = "test.c"
+  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"
+  
+  @str = private unnamed_addr constant [5 x i8] c"ohai\00", align 1
+  
+  ; Function Attrs: nounwind uwtable
+  define dso_local i32 @foo(i64 noundef %a, i64 noundef %b) local_unnamed_addr #0 !dbg !16 {
+  entry:
+    call void @llvm.dbg.value(metadata i64 %a, metadata !22, metadata !DIExpression()), !dbg !24
+    call void @llvm.dbg.value(metadata i64 %b, metadata !23, metadata !DIExpression()), !dbg !24
+    %call = tail call i32 (...) @baz() #5, !dbg !25
+    %conv = trunc i64 %a to i32, !dbg !26
+    %conv1 = trunc i64 %b to i32, !dbg !27
+    %call2 = tail call i32 @bar(i32 noundef %conv, i32 noundef %conv1) #5, !dbg !28
+    ret i32 %call2, !dbg !29
+  }
+  
+  declare !dbg !30 i32 @baz(...) local_unnamed_addr #1
+  
+  declare !dbg !34 i32 @bar(i32 noundef, i32 noundef) local_unnamed_addr #1
+  
+  ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #3
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!9, !10, !11, !12, !13, !14}
+  !llvm.ident = !{!15}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: ".", checksumkind: CSK_MD5, checksum: "98e51a9cf3bc6e1c3cc216ca4c82353a")
+  !2 = !{!3}
+  !3 = !DIGlobalVariableExpression(var: !4, expr: !DIExpression())
+  !4 = distinct !DIGlobalVariable(scope: null, file: !1, line: 11, type: !5, isLocal: true, isDefinition: true)
+  !5 = !DICompositeType(tag: DW_TAG_array_type, baseType: !6, size: 48, elements: !7)
+  !6 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+  !7 = !{!8}
+  !8 = !DISubrange(count: 6)
+  !9 = !{i32 7, !"Dwarf Version", i32 5}
+  !10 = !{i32 2, !"Debug Info Version", i32 3}
+  !11 = !{i32 1, !"wchar_size", i32 4}
+  !12 = !{i32 8, !"PIC Level", i32 2}
+  !13 = !{i32 7, !"PIE Level", i32 2}
+  !14 = !{i32 7, !"uwtable", i32 2}
+  !15 = !{!"clang"}
+  !16 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !17, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !21)
+  !17 = !DISubroutineType(types: !18)
+  !18 = !{!19, !20, !20}
+  !19 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !20 = !DIBasicType(name: "long", size: 64, encoding: DW_ATE_signed)
+  !21 = !{!22, !23}
+  !22 = !DILocalVariable(name: "a", arg: 1, scope: !16, file: !1, line: 5, type: !20)
+  !23 = !DILocalVariable(name: "b", arg: 2, scope: !16, file: !1, line: 5, type: !20)
+  !24 = !DILocation(line: 0, scope: !16)
+  !25 = !DILocation(line: 6, column: 3, scope: !16)
+  !26 = !DILocation(line: 7, column: 14, scope: !16)
+  !27 = !DILocation(line: 7, column: 17, scope: !16)
+  !28 = !DILocation(line: 7, column: 10, scope: !16)
+  !29 = !DILocation(line: 7, column: 3, scope: !16)
+  !30 = !DISubprogram(name: "baz", scope: !1, file: !1, line: 4, type: !31, spFlags: DISPFlagOptimized, retainedNodes: !33)
+  !31 = !DISubroutineType(types: !32)
+  !32 = !{!19}
+  !33 = !{}
+  !34 = !DISubprogram(name: "bar", scope: !1, file: !1, line: 3, type: !35, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !33)
+  !35 = !DISubroutineType(types: !36)
+  !36 = !{!19, !19, !19}
+  !37 = distinct !DISubprogram(name: "qux", scope: !1, file: !1, line: 10, type: !31, scopeLine: 10, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !33)
+  !38 = !DILocation(line: 11, column: 3, scope: !37)
+  !39 = !DILocation(line: 12, column: 3, scope: !37)
+
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$rdi' }
+  - { reg: '$rsi' }
+frameInfo:
+  stackSize:       24
+  offsetAdjustment: -24
+  maxAlignment:    1
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+  cvBytesOfCalleeSavedRegisters: 16
+  hasTailCall:     true
+fixedStack:
+  - { id: 0, type: spill-slot, offset: -24, size: 8, alignment: 8, callee-saved-register: '$rbx' }
+  - { id: 1, type: spill-slot, offset: -16, size: 8, alignment: 16, callee-saved-register: '$r14' }
+callSites:
+  - { bb: 0, offset: 15 }
+  - { bb: 0, offset: 28, fwdArgRegs: 
+      - { arg: 0, reg: '$edi' }
+      - { arg: 1, reg: '$esi' } }
+debugValueSubstitutions:
+  - { srcinst: 2, srcop: 0, dstinst: 1, dstop: 0, subreg: 6 }
+  - { srcinst: 4, srcop: 0, dstinst: 3, dstop: 0, subreg: 6 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $rdi, $rsi, $r14, $rbx
+  
+    DBG_VALUE $rdi, $noreg, !22, !DIExpression(), debug-location !24
+    DBG_VALUE $rsi, $noreg, !23, !DIExpression(), debug-location !24
+    frame-setup PUSH64r killed $r14, implicit-def $rsp, implicit $rsp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 16
+    frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 24
+    frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 32
+    CFI_INSTRUCTION offset $rbx, -24
+    CFI_INSTRUCTION offset $r14, -16
+    DBG_PHI $rsi, 3
+    DBG_PHI $rdi, 1
+    $rbx = MOV64rr $rsi
+    $r14 = MOV64rr $rdi
+    dead $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $al, debug-location !25
+    CALL64pcrel32 target-flags(x86-plt) @baz, csr_64, implicit $rsp, implicit $ssp, implicit killed $al, implicit-def $rsp, implicit-def $ssp, implicit-def dead $eax, debug-location !25
+    DBG_VALUE $rbx, $noreg, !23, !DIExpression(), debug-location !24
+    DBG_VALUE $r14, $noreg, !22, !DIExpression(), debug-location !24
+    $edi = MOV32rr $r14d, implicit killed $r14, debug-location !28
+    $esi = MOV32rr $ebx, implicit killed $rbx, debug-location !28
+    $rsp = frame-destroy ADD64ri8 $rsp, 8, implicit-def dead $eflags, debug-location !28
+    frame-destroy CFI_INSTRUCTION def_cfa_offset 24, debug-location !28
+      ;; This tail-call popping of rbx clobbers the call site information
+    $rbx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !28
+    DBG_VALUE $rsi, $noreg, !23, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location !24
+    frame-destroy CFI_INSTRUCTION def_cfa_offset 16, debug-location !28
+      ;; This tail-call popping of r14 clobbers the call site information
+    $r14 = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !28
+    DBG_VALUE $rdi, $noreg, !22, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location !24
+    frame-destroy CFI_INSTRUCTION def_cfa_offset 8, debug-location !28
+    TAILJMPd64 target-flags(x86-plt) @bar, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit $edi, implicit $esi, debug-location !28
+
+...