[DebugInfo][InstrRef] Cope with win32 calls changing the stack-pointer in LiveDebugValues

[jmorse]

Summary

This patch works around an edge case in win32 environments that could produce false addresses for variable length arrays.

Most of LiveDebugValues (VarLoc and InstrRef) is written with the assumption that register defs of the stack pointer on call instructions should be ignored -- the call instruction certainly alters SP, but the stack is balanced when the call returns [0]. Unfortunately there's a single scenario where this definitely isn't true: on 32-bit Windows, with variable-length arrays, we call a function _chkstk that both probes the stack and alters the stack pointer. With DBG_VALUE based variable locations, this isn't a huge problem: a DBG_VALUE is applied to the stack pointer after the function returns, and is then copied to other registers. However in instruction referencing mode, we need to instrument the instruction defining the value (see next patch): that means the call to _chkstk, which violates the above assumption that calls don't define the stack pointer.

This patch adds an exception: we add a target frame-lowering hook to see whether stack probe functions will modify the stack pointer, store that in an internal flag, and if it's true then scan CALL instructions to see whether they're such badly-behaved functions. If they are, we recognise them as defining a new stack-pointer value. If we don't do this, then LiveDebugValues will always think "the dynamic allocas address is always whatever the current stack pointer is", which is going to produce false locations if more than one dynamic alloca is used.

The added test exercises this behaviour: two calls to _chkstk should be considered as producing two different values.

[0] This isn't true of stdcall, but I think we have existing difficulties with that anyway.

[Orlando]

I'm definitely not a windows expert by any stretch, so wouldn't object to a second opinion on this and the follow-up patch. But everything you've said makes sense and the changes LGTM (with a few inline nits).

[0] This isn't true of stdcall, but I think we have existing difficulties with that anyway.

😬

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1291–1306

Any reason not to stuff this all inside the IgnoreSPAlias lambda? Not a strong opinion.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

822

nit: does- > do

825

nit: target's

[jmorse]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1291–1306

Performance considerations -- the lambda might be called a lot, wouldn't want to use strcmp repeatedly when it can instead be used once.

llvm/include/llvm/CodeGen/TargetFrameLowering.h

public:
   virtual void
   emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI) const {}
 
   /// Replace a StackProbe stub (if any) with the actual probe code inline
   virtual void inlineStackProbe(MachineFunction &MF,
                                 MachineBasicBlock &PrologueMBB) const {}
 
+  /// Does the stack probe function call return with a modified stack pointer?
+  virtual bool stackProbeFunctionModifiesSP() const { return false; }
+
   /// Adjust the prologue to have the function use segmented stacks. This works
   /// by adding a check even before the "normal" function prologue.
   virtual void adjustForSegmentedStacks(MachineFunction &MF,
                                         MachineBasicBlock &PrologueMBB) const {}
 
   /// Adjust the prologue to add Erlang Run-Time System (ERTS) specific code in
   /// the assembly prologue to explicitly handle the stack.
   virtual void adjustForHiPEPrologue(MachineFunction &MF,

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

private:
   /// location problem -- we use LLVMs SSA Updater to fix changes by
   /// optimizations that destroy PHI instructions.
   SmallVector<DebugPHIRecord, 32> DebugPHINumToValue;
 
   // Map of overlapping variable fragments.
   OverlapMap OverlapFragments;
   VarToFragments SeenFragments;
 
+  /// True if we need to examine call instructions for stack clobbers. We
+  /// normally assume that they don't clobber SP, but stack probes on Windows
+  /// do.
+  bool AdjustsStackInCalls = false;
+
+  /// If AdjustsStackInCalls is true, this holds the name of the target's stack
+  /// probe function, which is the function we expect will alter the stack
+  /// pointer.
+  StringRef StackProbeSymbolName;
+
   /// Tests whether this instruction is a spill to a stack slot.
   bool isSpillInstruction(const MachineInstr &MI, MachineFunction *MF);
 
   /// Decide if @MI is a spill instruction and return true if it is. We use 2
   /// criteria to make this decision:
   /// - Is this instruction a store to a spill slot?
   /// - Is there a register operand that is both used and killed?
   /// TODO: Store optimization can fold spills into other stores (including

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

if (MI.isImplicitDef()) {
     ValueIDNum Num = MTracker->readReg(MI.getOperand(0).getReg());
     // Has a legitimate value -> ignore the implicit def.
     if (Num.getLoc() != 0)
       return;
     // Otherwise, def it here.
   } else if (MI.isMetaInstruction())
     return;
 
-  auto IgnoreSPAlias = [this, &MI](Register R) -> bool {
+  // We always ignore SP defines on call instructions, they don't actually
+  // change the value of the stack pointer... except for win32's _chkstk. This
+  // is rare: filter quickly for the common case (no stack adjustments, not a
+  // call, etc). If it is a call that modifies SP, recognise the SP register
+  // defs.
+  bool CallChangesSP = false;
+  if (AdjustsStackInCalls && MI.isCall() && MI.getOperand(0).isSymbol() &&
+      !strcmp(MI.getOperand(0).getSymbolName(), StackProbeSymbolName.data()))
+    CallChangesSP = true;
+
+  // Test whether we should ignore a def of this register due to it being part
+  // of the stack pointer.
+  auto IgnoreSPAlias = [this, &MI, CallChangesSP](Register R) -> bool {
+    if (CallChangesSP)
+      return false;
     return MI.isCall() && MTracker->SPAliases.count(R);
   };
 
   // Find the regs killed by MI, and find regmasks of preserved regs.
   // Max out the number of statically allocated elements in `DeadRegs`, as this
   // prevents fallback to std::set::count() operations.
   SmallSet<uint32_t, 32> DeadRegs;
   SmallVector<const uint32_t *, 4> RegMasks;
   SmallVector<const MachineOperand *, 4> RegMaskPtrs;
bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   TRI = MF.getSubtarget().getRegisterInfo();
   MRI = &MF.getRegInfo();
   TII = MF.getSubtarget().getInstrInfo();
   TFI = MF.getSubtarget().getFrameLowering();
   TFI->getCalleeSaves(MF, CalleeSavedRegs);
   MFI = &MF.getFrameInfo();
   LS.initialize(MF);
 
+  const auto &STI = MF.getSubtarget();
+  AdjustsStackInCalls = MFI->adjustsStack() &&
+                        STI.getFrameLowering()->stackProbeFunctionModifiesSP();
+  if (AdjustsStackInCalls)
+    StackProbeSymbolName = STI.getTargetLowering()->getStackProbeSymbolName(MF);
+
   MTracker =
       new MLocTracker(MF, *TII, *TRI, *MF.getSubtarget().getTargetLowering());
   VTracker = nullptr;
   TTracker = nullptr;
 
   SmallVector<MLocTransferMap, 32> MLocTransfer;
   SmallVector<VLocTracker, 8> vlocs;
   LiveInsT SavedLiveIns;

llvm/lib/Target/X86/X86FrameLowering.h

public:
 
   /// Emit target stack probe code. This is required for all
   /// large stack allocations on Windows. The caller is required to materialize
   /// the number of bytes to probe in RAX/EAX.
   void emitStackProbe(MachineFunction &MF, MachineBasicBlock &MBB,
                       MachineBasicBlock::iterator MBBI, const DebugLoc &DL,
                       bool InProlog) const;
 
+  bool stackProbeFunctionModifiesSP() const override;
+
   /// Replace a StackProbe inline-stub with the actual probe code inline.
   void inlineStackProbe(MachineFunction &MF,
                         MachineBasicBlock &PrologMBB) const override;
 
   void
   emitCalleeSavedFrameMoves(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI) const override;
 

llvm/lib/Target/X86/X86FrameLowering.cpp

if (STI.isTargetWindowsCoreCLR()) {
     } else {
       emitStackProbeInline(MF, MBB, MBBI, DL, false);
     }
   } else {
     emitStackProbeCall(MF, MBB, MBBI, DL, InProlog);
   }
 }
 
+bool X86FrameLowering::stackProbeFunctionModifiesSP() const {
+  return STI.isOSWindows() && !STI.isTargetWin64();
+}
+
 void X86FrameLowering::inlineStackProbe(MachineFunction &MF,
                                         MachineBasicBlock &PrologMBB) const {
   auto Where = llvm::find_if(PrologMBB, [](MachineInstr &MI) {
     return MI.getOpcode() == X86::STACKALLOC_W_PROBING;
   });
   if (Where != PrologMBB.end()) {
     DebugLoc DL = PrologMBB.findDebugLoc(Where);
     emitStackProbeInline(MF, PrologMBB, Where, DL, true);

llvm/test/DebugInfo/MIR/InstrRef/win32-chkctk-modifies-esp.mir

+# RUN: llc %s -o - -run-pass=livedebugvalues \
+# RUN:      -experimental-debug-variable-locations \
+# RUN:  | FileCheck %s
+#
+## Copy of DebugInfo/COFF/types-array-advanced.ll, altered to test its behaviour
+## in MIR when fed to LiveDebugValues. The matter of interest is that on win32,
+## calls to _chkstk will modify $esp, where we would normally ignore defs of the
+## stack pointer on calls.
+##
+## Check that on win32, for this specific symbol, when the stack is adjusted,
+## that we track the modified $esp from _chkstk like any other value in
+## LiveDebugValues, where it can be recovered after a clobber or moved around.
+## Check lines written inline.
+--- |
+  target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:32-n8:16:32-a:0:32-S32"
+  target triple = "i686-pc-windows-msvc18.0.31101"
+  
+  %struct.incomplete_struct = type { i32 }
+  
+  @"\01?multi_dim_arr@@3PAY146DA" = global [2 x [5 x [7 x i8]]] zeroinitializer, align 1, !dbg !0
+  @"\01?p_incomplete_struct_arr@@3PAY02Uincomplete_struct@@A" = global [3 x i8]* null, align 4, !dbg !6
+  @"\01?incomplete_struct_arr@@3PAUincomplete_struct@@A" = global [3 x %struct.incomplete_struct] zeroinitializer, align 4, !dbg !16
+  @"\01?typedef_arr@@3SDHD" = constant [4 x i32] zeroinitializer, align 4, !dbg !18
+  
+  define void @"\01?foo@@YAXH@Z"(i32 %x) !dbg !35 {
+  entry:
+    %x.addr = alloca i32, align 4
+    %saved_stack = alloca i8*, align 4
+    store i32 %x, i32* %x.addr, align 4
+    call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !38, metadata !DIExpression()), !dbg !39
+    %0 = load i32, i32* %x.addr, align 4, !dbg !40
+    %1 = call i8* @llvm.stacksave(), !dbg !41
+    store i8* %1, i8** %saved_stack, align 4, !dbg !41
+    %vla = alloca i32, i32 %0, align 4, !dbg !41
+    call void @llvm.dbg.declare(metadata i32* %vla, metadata !42, metadata !DIExpression(DW_OP_deref)), !dbg !46
+    %arrayidx1 = bitcast i32* %vla to i32*, !dbg !47
+    store i32 0, i32* %arrayidx1, align 4, !dbg !48
+    %2 = load i8*, i8** %saved_stack, align 4, !dbg !49
+    call void @llvm.stackrestore(i8* %2), !dbg !49
+    ret void, !dbg !49
+  }
+  
+  ; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.declare(metadata, metadata, metadata)
+  
+  ; Function Attrs: nofree nosync nounwind willreturn
+  declare i8* @llvm.stacksave()
+  
+  ; Function Attrs: nofree nosync nounwind willreturn
+  declare void @llvm.stackrestore(i8*)
+  
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!32, !33}
+  !llvm.ident = !{!34}
+  
+  !0 = distinct !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+  !1 = !DIGlobalVariable(name: "multi_dim_arr", linkageName: "\01?multi_dim_arr@@3PAY146DA", scope: !2, file: !3, line: 1, type: !26, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 3.9.0 (trunk 273874)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+  !3 = !DIFile(filename: "t.cpp", directory: "/")
+  !4 = !{}
+  !5 = !{!0, !6, !16, !18}
+  !6 = distinct !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+  !7 = !DIGlobalVariable(name: "p_incomplete_struct_arr", linkageName: "\01?p_incomplete_struct_arr@@3PAY02Uincomplete_struct@@A", scope: !2, file: !3, line: 3, type: !8, isLocal: false, isDefinition: true)
+  !8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 32, align: 32)
+  !9 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, elements: !14)
+  !10 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "incomplete_struct", file: !3, line: 4, size: 32, align: 32, elements: !11, identifier: ".?AUincomplete_struct@@")
+  !11 = !{!12}
+  !12 = !DIDerivedType(tag: DW_TAG_member, name: "s1", scope: !10, file: !3, line: 5, baseType: !13, size: 32, align: 32)
+  !13 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+  !14 = !{!15}
+  !15 = !DISubrange(count: 3)
+  !16 = distinct !DIGlobalVariableExpression(var: !17, expr: !DIExpression())
+  !17 = !DIGlobalVariable(name: "incomplete_struct_arr", linkageName: "\01?incomplete_struct_arr@@3PAUincomplete_struct@@A", scope: !2, file: !3, line: 6, type: !9, isLocal: false, isDefinition: true)
+  !18 = distinct !DIGlobalVariableExpression(var: !19, expr: !DIExpression())
+  !19 = !DIGlobalVariable(name: "typedef_arr", linkageName: "\01?typedef_arr@@3SDHD", scope: !2, file: !3, line: 14, type: !20, isLocal: false, isDefinition: true)
+  !20 = !DICompositeType(tag: DW_TAG_array_type, baseType: !21, size: 128, align: 32, elements: !24)
+  !21 = !DIDerivedType(tag: DW_TAG_typedef, name: "T_INT", file: !3, line: 13, baseType: !22)
+  !22 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !23)
+  !23 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !13)
+  !24 = !{!25}
+  !25 = !DISubrange(count: 4)
+  !26 = !DICompositeType(tag: DW_TAG_array_type, baseType: !27, size: 560, align: 8, elements: !28)
+  !27 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
+  !28 = !{!29, !30, !31}
+  !29 = !DISubrange(count: 2)
+  !30 = !DISubrange(count: 5)
+  !31 = !DISubrange(count: 7)
+  !32 = !{i32 2, !"CodeView", i32 1}
+  !33 = !{i32 2, !"Debug Info Version", i32 3}
+  !34 = !{!"clang version 3.9.0 (trunk 273874)"}
+  !35 = distinct !DISubprogram(name: "foo", linkageName: "\01?foo@@YAXH@Z", scope: !3, file: !3, line: 8, type: !36, scopeLine: 8, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !4)
+  !36 = !DISubroutineType(types: !37)
+  !37 = !{null, !13}
+  !38 = !DILocalVariable(name: "x", arg: 1, scope: !35, file: !3, line: 8, type: !13)
+  !39 = !DILocation(line: 8, column: 14, scope: !35)
+  !40 = !DILocation(line: 9, column: 21, scope: !35)
+  !41 = !DILocation(line: 9, column: 4, scope: !35)
+  !42 = !DILocalVariable(name: "dyn_size_arr", scope: !35, file: !3, line: 9, type: !43)
+  !43 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, align: 32, elements: !44)
+  !44 = !{!45}
+  !45 = !DISubrange(count: -1)
+  !46 = !DILocation(line: 9, column: 8, scope: !35)
+  !47 = !DILocation(line: 10, column: 4, scope: !35)
+  !48 = !DILocation(line: 10, column: 20, scope: !35)
+  !49 = !DILocation(line: 11, column: 1, scope: !35)
+
+...
+---
+name:            "\x01?foo@@YAXH@Z"
+alignment:       16
+tracksRegLiveness: true
+hasWinCFI:       true
+frameInfo:
+  stackSize:       8
+  offsetAdjustment: -8
+  maxAlignment:    4
+  adjustsStack:    true
+  maxCallFrameSize: 0
+fixedStack:
+  - { id: 0, type: spill-slot, offset: -8, size: 4, alignment: 4 }
+  - { id: 1, size: 4, alignment: 4, debug-info-variable: '!38', debug-info-expression: '!DIExpression()', 
+      debug-info-location: '!39' }
+stack:
+  - { id: 1, name: saved_stack, offset: -12, size: 4, alignment: 4 }
+  - { id: 2, name: vla, type: variable-sized, offset: -12, alignment: 1 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    frame-setup PUSH32r killed $ebp, implicit-def $esp, implicit $esp
+    CFI_INSTRUCTION def_cfa_offset 8
+    CFI_INSTRUCTION offset $ebp, -8
+    frame-setup SEH_PushReg 23
+    $ebp = frame-setup MOV32rr $esp
+    CFI_INSTRUCTION def_cfa_register $ebp
+    frame-setup SEH_SetFrame 23, 0
+    frame-setup PUSH32r undef $eax, implicit-def $esp, implicit $esp
+    frame-setup SEH_StackAlloc 4
+    frame-setup SEH_EndPrologue
+    renamable $eax = MOV32rm $ebp, 1, $noreg, 8, $noreg, debug-location !40 :: (dereferenceable load (s32) from %ir.x.addr)
+    MOV32mr $ebp, 1, $noreg, -4, $noreg, $esp, debug-location !41 :: (store (s32) into %ir.saved_stack)
+    renamable $eax = SHL32ri killed renamable $eax, 2, implicit-def dead $eflags, debug-location !41
+    CALLpcrel32 &_chkstk, implicit $esp, implicit $ssp, implicit $eax, implicit $esp, implicit-def dead $eax, implicit-def $esp, implicit-def dead $eflags, debug-instr-number 2, debug-location !41
+    $ebx = MOV32rr $esp, debug-location !41
+    $eax = MOV32ri 0
+    DBG_INSTR_REF 2, 6, !42, !DIExpression(DW_OP_deref), debug-location !46
+    ; CHECK-LABEL: DBG_INSTR_REF 2, 6
+    ; CHECK:       DBG_VALUE $esp
+
+    ;; Variable value is $esp / $ebx, will be based on $esp initially. We'll now
+    ;; allocate more stack space, and several things should happen:
+    ;;  * We recognise it as modifying $esp, and move the variable location to
+    ;;    be based on $ebx,
+    ;;  * We recognise the modified $esp as a new value, and if we assign it to
+    ;;    the variable location, will base the variable on $esp,
+    ;;  * Clobbering the second modified $esp won't re-locate the variable to
+    ;;    $ebx, which comes from the first modified $esp.
+    CALLpcrel32 &_chkstk, implicit $esp, implicit $ssp, implicit $eax, implicit $esp, implicit-def dead $eax, implicit-def $esp, implicit-def dead $eflags, debug-instr-number 3, debug-location !41
+    ; CHECK-NEXT: CALLpcrel32
+    ; CHECK-NEXT: DBG_VALUE $ebx
+
+    DBG_INSTR_REF 3, 6, !42, !DIExpression(DW_OP_deref), debug-location !46
+    ; CHECK-NEXT: DBG_INSTR_REF 3, 6
+    ; CHECK-NEXT: DBG_VALUE $esp
+
+    $esp = ADD32ri killed $esp, 0, implicit-def dead $eflags
+    ; CHECK-NEXT: ADD32ri
+
+    DBG_INSTR_REF 3, 6, !42, !DIExpression(DW_OP_deref), debug-location !46
+    ; CHECK-NEXT: DBG_INSTR_REF 3, 6
+    ; CHECK-NEXT: DBG_VALUE $noreg
+
+    $esp = MOV32rr $ebp, debug-location !49
+    $ebp = frame-destroy POP32r implicit-def $esp, implicit $esp, debug-location !49
+    RET32 debug-location !49
+
+...