[DebugInfo][InstrRef] Avoid a crash from mixed variable location modes

[jmorse]

Summary

Variable locations now come in two modes, instruction referencing and DBG_VALUE -- I initially assumed that we could use information like the current optimisation level to detect which mode we should use (-O0 uses DBG_VALUE). Unfortunately, it turns out that SelectionDAG fiddles with the optimisation level in the presence of opt-bisect-limit (see the lines in SelectionDAGISel::runOnMachineFunction immediately below my additions), making this not a robust technique. Over in [0] there's a crash observed from this, SelectionDAG produces DBG_VALUEs but the debug-info passes expect to see DBG_INSTR_REFs.

Changing the target options in the middle of a pass is accepted as a horrible hack in the comments in SelectionDAG -- there's not much to do about this, except:

• Store a flag in MachineFunction indicating what flavour of debug-info it contains,
• Plumb the original debug-info flavour through SelectionDAG so that the correct one is picked.

This patch implements the latter: the "use instr-ref?" boolean is stored before the target options are edited, then passed down to the instruction emitting functions. This is mildly ugly, but there aren't any good options.

[0] https://github.com/llvm/llvm-project/issues/53937

[Orlando]

I wonder if the test IR could be simplified. Would it not be enough to have a function with a single dbg.value and nothing else?

LGTM either way.

[jmorse]

Dropping in with the addition of a null-check in SelectionDAGISel::SelectAllBasicBlocks, not all targets actaully create a FastISel instance when createFastISel is called.

llvm/include/llvm/CodeGen/FastISel.h

protected:
   DebugLoc DbgLoc;
   const TargetMachine &TM;
   const DataLayout &DL;
   const TargetInstrInfo &TII;
   const TargetLowering &TLI;
   const TargetRegisterInfo &TRI;
   const TargetLibraryInfo *LibInfo;
   bool SkipTargetIndependentISel;
+  bool UseInstrRefDebugInfo = false;
 
   /// The position of the last instruction for materializing constants
   /// for use in the current block. It resets to EmitStartPt when it makes sense
   /// (for example, it's usually profitable to avoid function calls between the
   /// definition and the use)
   MachineInstr *LastLocalValue = nullptr;
 
   /// The top most instruction in the current block that is allowed for
public:
 
   /// Prepare InsertPt to begin inserting instructions into the local
   /// value area and return the old insert position.
   SavePoint enterLocalValueArea();
 
   /// Reset InsertPt to the given old insert position.
   void leaveLocalValueArea(SavePoint Old);
 
+  /// Signal whether instruction referencing variable locations are desired for
+  /// this function's debug-info.
+  void useInstrRefDebugInfo(bool Flag) {
+    UseInstrRefDebugInfo = Flag;
+  }
+
 protected:
   explicit FastISel(FunctionLoweringInfo &FuncInfo,
                     const TargetLibraryInfo *LibInfo,
                     bool SkipTargetIndependentISel = false);
 
   /// This method is called by target-independent code when the normal
   /// FastISel process fails to select an instruction. This gives targets a
   /// chance to emit code for anything that doesn't fit into FastISel's

llvm/include/llvm/CodeGen/SelectionDAG.h

struct CallSiteDbgInfo {
     MDNode *HeapAllocSite = nullptr;
     bool NoMerge = false;
   };
 
   DenseMap<const SDNode *, CallSiteDbgInfo> SDCallSiteDbgInfo;
 
   uint16_t NextPersistentId = 0;
 
+  /// Are instruction referencing variable locations desired for this function?
+  bool UseInstrRefDebugInfo = false;
+
 public:
   /// Clients of various APIs that cause global effects on
   /// the DAG can optionally implement this interface.  This allows the clients
   /// to handle the various sorts of updates that happen.
   ///
   /// A DAGUpdateListener automatically registers itself with DAG when it is
   /// constructed, and removes itself when destroyed in RAII fashion.
   struct DAGUpdateListener {
public:
   SDDbgInfo::DbgLabelIterator DbgLabelEnd() const {
     return DbgInfo->DbgLabelEnd();
   }
 
   /// To be invoked on an SDNode that is slated to be erased. This
   /// function mirrors \c llvm::salvageDebugInfo.
   void salvageDebugInfo(SDNode &N);
 
+  /// Signal whether instruction referencing variable locations are desired for
+  /// this function's debug-info.
+  void useInstrRefDebugInfo(bool Flag) {
+    UseInstrRefDebugInfo = Flag;
+  }
+
+  bool getUseInstrRefDebugInfo() const {
+    return UseInstrRefDebugInfo;
+  }
+
   void dump() const;
 
   /// In most cases this function returns the ABI alignment for a given type,
   /// except for illegal vector types where the alignment exceeds that of the
   /// stack. In such cases we attempt to break the vector down to a legal type
   /// and return the ABI alignment for that instead.
   Align getReducedAlign(EVT VT, bool UseABI);
 

llvm/include/llvm/CodeGen/SelectionDAGISel.h

public:
   std::unique_ptr<SelectionDAGBuilder> SDB;
   AAResults *AA = nullptr;
   GCFunctionInfo *GFI = nullptr;
   CodeGenOpt::Level OptLevel;
   const TargetInstrInfo *TII;
   const TargetLowering *TLI;
   bool FastISelFailed;
   SmallPtrSet<const Instruction *, 4> ElidedArgCopyInstrs;
+  bool UseInstrRefDebugInfo = false;
 
   /// Current optimization remark emitter.
   /// Used to report things like combines and FastISel failures.
   std::unique_ptr<OptimizationRemarkEmitter> ORE;
 
   static char ID;
 
   explicit SelectionDAGISel(TargetMachine &tm,

llvm/lib/CodeGen/MachineFunction.cpp

void MachineFunction::finalizeDebugInstrRefs() {
 
   auto MakeUndefDbgValue = [&](MachineInstr &MI) {
     const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE);
     MI.setDesc(RefII);
     MI.getOperand(0).setReg(0);
     MI.getOperand(1).ChangeToRegister(0, false);
   };
 
-  if (!useDebugInstrRef())
-    return;
-
   for (auto &MBB : *this) {
     for (auto &MI : MBB) {
       if (!MI.isDebugRef() || !MI.getOperand(0).isReg())
         continue;
 
       Register Reg = MI.getOperand(0).getReg();
 
       // Some vregs can be deleted as redundant in the meantime. Mark those

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

if (Op) {
       auto Builder =
           BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
                   TII.get(TargetOpcode::DBG_VALUE), /*IsIndirect*/ true, *Op,
                   DI->getVariable(), DI->getExpression());
 
       // If using instruction referencing, mutate this into a DBG_INSTR_REF,
       // to be later patched up by finalizeDebugInstrRefs. Tack a deref onto
       // the expression, we don't have an "indirect" flag in DBG_INSTR_REF.
-      if (FuncInfo.MF->useDebugInstrRef() && Op->isReg()) {
+      if (UseInstrRefDebugInfo && Op->isReg()) {
         Builder->setDesc(TII.get(TargetOpcode::DBG_INSTR_REF));
         Builder->getOperand(1).ChangeToImmediate(0);
         auto *NewExpr =
            DIExpression::prepend(DI->getExpression(), DIExpression::DerefBefore);
         Builder->getOperand(3).setMetadata(NewExpr);
       }
     } else {
       // We can't yet handle anything else here because it would require
if (!V || isa<UndefValue>(V) || DI->hasArgList()) {
       // FIXME: This does not handle register-indirect values at offset 0.
       bool IsIndirect = false;
       auto Builder =
           BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II, IsIndirect, Reg,
                   DI->getVariable(), DI->getExpression());
 
       // If using instruction referencing, mutate this into a DBG_INSTR_REF,
       // to be later patched up by finalizeDebugInstrRefs.
-      if (FuncInfo.MF->useDebugInstrRef()) {
+      if (UseInstrRefDebugInfo) {
         Builder->setDesc(TII.get(TargetOpcode::DBG_INSTR_REF));
         Builder->getOperand(1).ChangeToImmediate(0);
       }
     } else {
       // We don't know how to handle other cases, so we drop.
       LLVM_DEBUG(dbgs() << "Dropping debug info for " << *DI << "\n");
     }
     return true;

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.h

public:
   MachineBasicBlock *getBlock() { return MBB; }
 
   /// getInsertPos - Return the current insertion position.
   MachineBasicBlock::iterator getInsertPos() { return InsertPos; }
 
   /// InstrEmitter - Construct an InstrEmitter and set it to start inserting
   /// at the given position in the given block.
   InstrEmitter(const TargetMachine &TM, MachineBasicBlock *mbb,
-               MachineBasicBlock::iterator insertpos);
+               MachineBasicBlock::iterator insertpos,
+               bool UseInstrRefDebugInfo);
 
 private:
   void EmitMachineNode(SDNode *Node, bool IsClone, bool IsCloned,
                        DenseMap<SDValue, Register> &VRBaseMap);
   void EmitSpecialNode(SDNode *Node, bool IsClone, bool IsCloned,
                        DenseMap<SDValue, Register> &VRBaseMap);
 };
 } // namespace llvm
 
 #endif

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

case ISD::INLINEASM_BR: {
     break;
   }
   }
 }
 
 /// InstrEmitter - Construct an InstrEmitter and set it to start inserting
 /// at the given position in the given block.
 InstrEmitter::InstrEmitter(const TargetMachine &TM, MachineBasicBlock *mbb,
-                           MachineBasicBlock::iterator insertpos)
+                           MachineBasicBlock::iterator insertpos,
+                           bool UseInstrRefDebugInfo)
     : MF(mbb->getParent()), MRI(&MF->getRegInfo()),
       TII(MF->getSubtarget().getInstrInfo()),
       TRI(MF->getSubtarget().getRegisterInfo()),
       TLI(MF->getSubtarget().getTargetLowering()), MBB(mbb),
       InsertPos(insertpos) {
-  EmitDebugInstrRefs = MF->useDebugInstrRef();
+  EmitDebugInstrRefs = UseInstrRefDebugInfo;
 }

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

void ScheduleDAGLinearize::Schedule() {
   }
 
   Sequence.reserve(DAGSize);
   ScheduleNode(DAG->getRoot().getNode());
 }
 
 MachineBasicBlock*
 ScheduleDAGLinearize::EmitSchedule(MachineBasicBlock::iterator &InsertPos) {
-  InstrEmitter Emitter(DAG->getTarget(), BB, InsertPos);
+  InstrEmitter Emitter(DAG->getTarget(), BB, InsertPos,
+                       DAG->getUseInstrRefDebugInfo());
   DenseMap<SDValue, Register> VRBaseMap;
 
   LLVM_DEBUG({ dbgs() << "\n*** Final schedule ***\n"; });
 
   unsigned NumNodes = Sequence.size();
   MachineBasicBlock *BB = Emitter.getBlock();
   for (unsigned i = 0; i != NumNodes; ++i) {
     SDNode *N = Sequence[NumNodes-i-1];

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

 }
 
 /// EmitSchedule - Emit the machine code in scheduled order. Return the new
 /// InsertPos and MachineBasicBlock that contains this insertion
 /// point. ScheduleDAGSDNodes holds a BB pointer for convenience, but this does
 /// not necessarily refer to returned BB. The emitter may split blocks.
 MachineBasicBlock *ScheduleDAGSDNodes::
 EmitSchedule(MachineBasicBlock::iterator &InsertPos) {
-  InstrEmitter Emitter(DAG->getTarget(), BB, InsertPos);
+  InstrEmitter Emitter(DAG->getTarget(), BB, InsertPos,
+                       DAG->getUseInstrRefDebugInfo());
   DenseMap<SDValue, Register> VRBaseMap;
   DenseMap<SUnit*, Register> CopyVRBaseMap;
   SmallVector<std::pair<unsigned, MachineInstr*>, 32> Orders;
   SmallSet<Register, 8> Seen;
   bool HasDbg = DAG->hasDebugValues();
 
   // Emit a node, and determine where its first instruction is for debuginfo.
   // Zero, one, or multiple instructions can be created when emitting a node.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

if (mf.getProperties().hasProperty(
     return false;
   // Do some sanity-checking on the command-line options.
   assert((!EnableFastISelAbort || TM.Options.EnableFastISel) &&
          "-fast-isel-abort > 0 requires -fast-isel");
 
   const Function &Fn = mf.getFunction();
   MF = &mf;
 
+  // Decide what flavour of variable location debug-info will be used, before
+  // we change the optimisation level.
+  UseInstrRefDebugInfo = mf.useDebugInstrRef();
+  CurDAG->useInstrRefDebugInfo(UseInstrRefDebugInfo);
+
   // Reset the target options before resetting the optimization
   // level below.
   // FIXME: This is a horrible hack and should be processed via
   // codegen looking at the optimization level explicitly when
   // it wants to look at it.
   TM.resetTargetOptions(Fn);
   // Reset OptLevel to None for optnone functions.
   CodeGenOpt::Level NewOptLevel = OptLevel;
if (LDI != LiveInMap.end()) {
         MachineBasicBlock::iterator Pos = CopyUseMI;
         EntryMBB->insertAfter(Pos, NewMI);
       }
     }
   }
 
   // For debug-info, in instruction referencing mode, we need to perform some
   // post-isel maintenence.
+  if (UseInstrRefDebugInfo)
     MF->finalizeDebugInstrRefs();
 
   // Determine if there are any calls in this machine function.
   MachineFrameInfo &MFI = MF->getFrameInfo();
   for (const auto &MBB : *MF) {
     if (MFI.hasCalls() && MF->hasInlineAsm())
       break;
 
     for (const auto &MI : MBB) {

 
 void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
   FastISelFailed = false;
   // Initialize the Fast-ISel state, if needed.
   FastISel *FastIS = nullptr;
   if (TM.Options.EnableFastISel) {
     LLVM_DEBUG(dbgs() << "Enabling fast-isel\n");
     FastIS = TLI->createFastISel(*FuncInfo, LibInfo);
+    if (FastIS)
+      FastIS->useInstrRefDebugInfo(UseInstrRefDebugInfo);
   }
 
   ReversePostOrderTraversal<const Function*> RPOT(&Fn);
 
   // Lower arguments up front. An RPO iteration always visits the entry block
   // first.
   assert(*RPOT.begin() == &Fn.getEntryBlock());
   ++NumEntryBlocks;

llvm/test/DebugInfo/X86/instr-ref-opt-bisect.ll

+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=0 \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=10 \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=20 \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=30 \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=40 \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -opt-bisect-limit=100 \
+; RUN:   | FileCheck %s
+;; Test fast-isel for good measure too,
+; RUN: llc %s -o - -stop-after=livedebugvalues -fast-isel=true \
+; RUN:   | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -fast-isel=true \
+; RUN:   -opt-bisect-limit=0 | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -fast-isel=true \
+; RUN:   -opt-bisect-limit=10 | FileCheck %s
+; RUN: llc %s -o - -stop-after=livedebugvalues -fast-isel=true \
+; RUN:   -opt-bisect-limit=100 | FileCheck %s
+
+; The function below should be optimised with the "default" optimisation level.
+; However, opt-bisect-limit causes SelectionDAG to change the target settings
+; for the duration of SelectionDAG. This can lead to variable locations created
+; in one mode, but the rest of the debug-info analyses expecting the other.
+; Test that opt-bisect-limit can be used without any assertions firing, and
+; that instruction referencing instructions are seen each time.
+;
+; The selection of bisect positions above are not picked meaningfully, the
+; pass order and positioning will change over time. The most important part
+; is that the bisect limit sits between SelectionDAG and debug-info passes at
+; some point.
+
+; CHECK: DBG_INSTR_REF
+; CHECK: DBG_PHI
+
+; ModuleID = '/tmp/test.cpp'
+source_filename = "/tmp/test.cpp"
+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"
+
+%class.Class = type { i8 }
+
+; Function Attrs: mustprogress uwtable
+define dso_local void @_Z4FuncP5Class(%class.Class* noundef %c) local_unnamed_addr !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata %class.Class* %c, metadata !15, metadata !DIExpression()), !dbg !16
+  %tobool.not = icmp eq %class.Class* %c, null, !dbg !17
+  br i1 %tobool.not, label %land.lhs.true, label %if.end, !dbg !19
+
+land.lhs.true:                                    ; preds = %entry
+  %call = call noundef zeroext i1 @_Z4Condv(), !dbg !20
+  call void @llvm.assume(i1 %call), !dbg !21
+  %call1 = call noalias noundef nonnull dereferenceable(1) i8* @_Znwm(i64 noundef 1), !dbg !22, !heapallocsite !12
+  %0 = bitcast i8* %call1 to %class.Class*, !dbg !22
+  call void @llvm.dbg.value(metadata %class.Class* %0, metadata !15, metadata !DIExpression()), !dbg !16
+  br label %if.end, !dbg !23
+
+if.end:                                           ; preds = %land.lhs.true, %entry
+  %c.addr.0 = phi %class.Class* [ %c, %entry ], [ %0, %land.lhs.true ]
+  call void @llvm.dbg.value(metadata %class.Class* %c.addr.0, metadata !15, metadata !DIExpression()), !dbg !16
+  call void @_Z11DoSomethingR5Class(%class.Class* noundef nonnull align 1 dereferenceable(1) %c.addr.0), !dbg !24
+  ret void, !dbg !25
+}
+
+declare !dbg !26 dso_local noundef zeroext i1 @_Z4Condv() local_unnamed_addr
+
+; Function Attrs: nobuiltin allocsize(0)
+declare dso_local noundef nonnull i8* @_Znwm(i64 noundef) local_unnamed_addr
+
+declare !dbg !30 dso_local void @_Z11DoSomethingR5Class(%class.Class* noundef nonnull align 1 dereferenceable(1)) local_unnamed_addr
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+; Function Attrs: inaccessiblememonly nocallback nofree nosync nounwind willreturn
+declare void @llvm.assume(i1 noundef)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/test.cpp", directory: ".")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 2}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "Func", linkageName: "_Z4FuncP5Class", scope: !8, file: !8, line: 6, type: !9, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !14)
+!8 = !DIFile(filename: "/tmp/test.cpp", directory: "")
+!9 = !DISubroutineType(types: !10)
+!10 = !{null, !11}
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
+!12 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "Class", file: !8, line: 3, size: 8, flags: DIFlagTypePassByValue, elements: !13, identifier: "_ZTS5Class")
+!13 = !{}
+!14 = !{!15}
+!15 = !DILocalVariable(name: "c", arg: 1, scope: !7, file: !8, line: 6, type: !11)
+!16 = !DILocation(line: 0, scope: !7)
+!17 = !DILocation(line: 7, column: 10, scope: !18)
+!18 = distinct !DILexicalBlock(scope: !7, file: !8, line: 7, column: 9)
+!19 = !DILocation(line: 7, column: 12, scope: !18)
+!20 = !DILocation(line: 7, column: 15, scope: !18)
+!21 = !DILocation(line: 7, column: 9, scope: !7)
+!22 = !DILocation(line: 8, column: 13, scope: !18)
+!23 = !DILocation(line: 8, column: 9, scope: !18)
+!24 = !DILocation(line: 10, column: 5, scope: !7)
+!25 = !DILocation(line: 11, column: 1, scope: !7)
+!26 = !DISubprogram(name: "Cond", linkageName: "_Z4Condv", scope: !8, file: !8, line: 5, type: !27, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !13)
+!27 = !DISubroutineType(types: !28)
+!28 = !{!29}
+!29 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!30 = !DISubprogram(name: "DoSomething", linkageName: "_Z11DoSomethingR5Class", scope: !8, file: !8, line: 4, type: !31, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !13)
+!31 = !DISubroutineType(types: !32)
+!32 = !{null, !33}
+!33 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !12, size: 64)