[DebugInfo] Allow non-stack_value variadic expressions and use in DBG_INSTR_REF

[StephenTozer]

Summary

Prior to this patch, variadic DIExpressions (i.e. ones that contain DW_OP_LLVM_arg) could only be created by salvaging debug values to create stack value expressions, resulting in a DBG_VALUE_LIST being created. As of the previous patch in this patch stack, DBG_INSTR_REF's syntax has been changed to match DBG_VALUE_LIST in preparation for supporting variadic expressions. This patch adds some minor changes needed to allow variadic expressions that aren't stack values to exist, and allows variadic expressions that are trivially reduceable to non-variadic expressions to be handled similarly to non-variadic expressions.

[StephenTozer]

Move test changes ahead into this patch.

[jmorse]

Generally looks good, I've got a couple of questions inline.

Are there any serious performance implications for, in the direction these patches are moving in, there being lots of non-empty DIExpressions compared to today when they're mostly empty? (I'm thinking we might need a static, pre-allocated one in LLVMContext, if every variable is going to allocate one).

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1140–1145

Just to check -- this is purely for printing the informative comments on assembly output, it isn't actually functional, yes?

llvm/lib/IR/DebugInfoMetadata.cpp

1503–1511

Just to confirm, the reason why this wasn't present before was because all variadic expressions had to be stack_value, yes?

llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir

13 ↗

(On Diff #463224)

Does this patch actually make some DBG_VALUEs become DBG_VALUE_LIST? I've missed that part, and see it's happening in various tests.

[jmorse]

(Lo and behold, I already asked that last question in D133929, never mind about that)

[StephenTozer]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1140–1145

Correct, this is just removing noise from the patch - even if it was an expression being used somewhere, it'd still have the same meaning either way, just formatted slightly differently.

llvm/lib/IR/DebugInfoMetadata.cpp

1503–1511

Correct - worth noting the only reason they "had" to be stack_value was because they're only naturally produced by salvaging operations that create stack_value expressions, not because there's some inherent incompatibility.

[StephenTozer]

Updated ISel and LDV to have very basic handling of "variadic" DBG_INSTR_REFs, and updated tests to use DW_OP_LLVM_arg.

[StephenTozer]

llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir

13 ↗

(On Diff #463224)

Missed this question - this patch changes DBG_INSTR_REF to have the same syntax as a DBG_VALUE_LIST, which includes having the variadic flag set, hence LDV will output the debug value produced from a DBG_INSTR_REF as a DBG_VALUE_LIST.

[StephenTozer]

Move some changes from an earlier patch into this one: Make entry value expressions containing only a leading DW_OP_LLVM_arg, 0 valid, as the changes in this patch enable such expressions. Also add the convertToVariadicExpression function, as this is the first patch in which it is used.

[jmorse]

Looks good (leaning largely on my prior review).

[StephenTozer]

Rebased onto latest master, added unit tests for convertToVariadicExpression.

[fdeazeve]

Sorry to bump this review, but I'm currently investigating a couple of bugs which I believe are a result of this patch.

One thing that seems off here is that now the definition of isEntryValue is incorrect:

bool DIExpression::isEntryValue() const {
  return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_LLVM_entry_value;
}

Also note that the CodeGen/AsmPrinter/DwarfExpression.cpp code for DwarfExpression::addExpression doesn't expect an EntryValue as anything but the first operand of the op list.

Still investigating how to handle those, but figured I'd give you a heads up in case you have ideas

[StephenTozer]

In D133926#4591953, @fdeazeve wrote:

Sorry to bump this review, but I'm currently investigating a couple of bugs which I believe are a result of this patch.

One thing that seems off here is that now the definition of isEntryValue is incorrect:

bool DIExpression::isEntryValue() const {
  return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_LLVM_entry_value;
}

Also note that the CodeGen/AsmPrinter/DwarfExpression.cpp code for DwarfExpression::addExpression doesn't expect an EntryValue as anything but the first operand of the op list.

Still investigating how to handle those, but figured I'd give you a heads up in case you have ideas

I'll take a quick look - off the top of my head, there should be an interface to treat !DIExpression(DW_OP_LLVM_arg, 0, <ops>) identically to !DIExpression(<ops>), and if it either doesn't exist or is only applicable to specific cases then it should be created/generalized.

[fdeazeve]

I'll take a quick look - off the top of my head, there should be an interface to treat !DIExpression(DW_OP_LLVM_arg, 0, <ops>) identically to !DIExpression(<ops>), and if it either doesn't exist or is only applicable to specific cases then it should be created/generalized.

Thank you!

Also note that the CodeGen/AsmPrinter/DwarfExpression.cpp code for DwarfExpression::addExpression doesn't expect an EntryValue as anything but the first operand of the op list.

I think the problem here is that, inside DwarfCompileUnit::constructVariableDIEImpl, we are now taking the *variadic path* which ends up calling addExpression.
More concretely, inside that function we have:

if (auto *DVal = DV.getValueLoc()) {
  if (!DVal->isVariadic()) {

We are taking the else path of that second if, i.e., we are treating things as variadic where we before we wouldn't?

(lldb) p DV.getValueLoc()->isVariadic()
(bool) $2 = true
(lldb) p DV.getValueLoc()->dump()
Loc = { reg=134 } !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 16, DW_OP_deref, DW_OP_deref)

Because of this, we end up calling DwarfExpression::addExpression, which crashes for that example because it doesn't expect an entry value.

[StephenTozer]

In D133926#4592629, @fdeazeve wrote:

Because of this, we end up calling DwarfExpression::addExpression, which crashes for that example because it doesn't expect an entry value.

Thanks for the further details - in summary, as part of the changes to DBG_INSTR_REF, we started using the variadic syntax for debug values that would normally have been non-variadic - there's no fundamental problem with this, but clearly some edges weren't fully ironed out. I'll submit a patch shortly with a fix - do you have a reproducer for the issue?

[fdeazeve]

In D133926#4594914, @StephenTozer wrote:

In D133926#4592629, @fdeazeve wrote:

Because of this, we end up calling DwarfExpression::addExpression, which crashes for that example because it doesn't expect an entry value.

Thanks for the further details - in summary, as part of the changes to DBG_INSTR_REF, we started using the variadic syntax for debug values that would normally have been non-variadic - there's no fundamental problem with this, but clearly some edges weren't fully ironed out. I'll submit a patch shortly with a fix - do you have a reproducer for the issue?

Yup, that makes perfect sense. Let me see if I can conjure a min repro and I'll come back here shortly.

[fdeazeve]

Could you give this a try? It repros for me with a llc test.ll.

target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx12.1.0"

declare void @llvm.dbg.value(metadata, metadata, metadata) #0

define swifttailcc void @"blah"(ptr swiftasync %0) !dbg !26 {
  %use = getelementptr i8, ptr %0, i64 9
  call void @llvm.dbg.value(metadata ptr %0, metadata !30, metadata !DIExpression(DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 16, DW_OP_deref, DW_OP_deref)), !dbg !34
  %use1 = load i32, ptr null, !dbg !45
  %use2 = sext i32 %use1 to i64
  %use3 = getelementptr i8, ptr null, i64 %use2
  store ptr %use3, ptr %0
  ret void
}

attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }

!llvm.module.flags = !{!0}
!llvm.dbg.cu = !{!1}

!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !7, producer: "blah", isOptimized: true, flags: "blah", runtimeVersion: 5, emissionKind: FullDebug, globals: !3, imports: !10, sysroot: "blah", sdk: "blah")
!3 = !{!4, !11}
!4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression())
!5 = distinct !DIGlobalVariable(name: "blah", linkageName: "blah", scope: !6, file: !7, line: 49, type: !8, isLocal: true, isDefinition: true)
!6 = !DIModule(scope: null, name: "blah", includePath: "blah")
!7 = !DIFile(filename: "blah", directory: "blah")
!8 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !9)
!9 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !6, file: !7, size: 64, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah")
!10 = !{}
!11 = !DIGlobalVariableExpression(var: !12, expr: !DIExpression())
!12 = distinct !DIGlobalVariable(name: "blah", linkageName: "blah", scope: !6, file: !7, line: 44, type: !14, isLocal: false, isDefinition: true)
!14 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !15)
!15 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !17, file: !7, size: 64, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah")
!17 = !DIModule(scope: null, name: "blah", configMacros: "blah", includePath: "blah")
!26 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, line: 115, type: !29, scopeLine: 117, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !10, thrownTypes: !10)
!28 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !6, file: !7, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah")
!29 = !DISubroutineType(types: !10)
!30 = !DILocalVariable(name: "blah", arg: 1, scope: !31, file: !7, line: 95, type: !33, flags: DIFlagArtificial)
!31 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, line: 95, type: !32, scopeLine: 95, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !10)
!32 = distinct !DISubroutineType(types: !10)
!33 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !28)
!34 = !DILocation(line: 95, column: 9, scope: !31, inlinedAt: !35)
!35 = distinct !DILocation(line: 0, scope: !36, inlinedAt: !38)
!36 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, type: !32, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1)
!38 = distinct !DILocation(line: 121, column: 36, scope: !44)
!44 = distinct !DILexicalBlock(scope: !26, file: !7, line: 116, column: 7)
!45 = !DILocation(line: 0, scope: !46, inlinedAt: !35)
!46 = !DILexicalBlockFile(scope: !31, file: !7, discriminator: 0)

[StephenTozer]

In D133926#4595209, @fdeazeve wrote:

Could you give this a try? It repros for me with a llc test.ll.

Repro is good, review opened: D158185

llvm/docs/MIRLangRef.rst

 A debug instruction reference operand is a pair of indices, referring to an
 instruction and an operand within that instruction respectively; see
 :ref:`Instruction referencing locations <instruction-referencing-locations>`.
 
 The example below uses a reference to Instruction 1, Operand 0:
 
 .. code-block:: text
 
-    DBG_INSTR_REF !123, !DIExpression(), dbg-instr-ref(1, 0), debug-location !456
+    DBG_INSTR_REF !123, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !456
 
 CFIIndex Operands
 ^^^^^^^^^^^^^^^^^
 
 A CFI Index operand is holding an index into a per-function side-table,
 ``MachineFunction::getFrameInstructions()``, which references all the frame
 instructions in a ``MachineFunction``. A ``CFI_INSTRUCTION`` may look like it
 contains multiple operands, but the only operand it contains is the CFI Index.

 *values* from the program point where a variable takes on that value. Changes
 in variable value occur in the same manner as ``DBG_VALUE`` meta instructions
 but using ``DBG_INSTR_REF``. Variable values are identified by a pair of
 instruction number and operand number. Consider the example below:
 
 .. code-block:: text
 
     $rbp = MOV64ri 0, debug-instr-number 1, debug-location !12
-    DBG_INSTR_REF !123, !DIExpression(), dbg-instr-ref(1, 0), debug-location !456
+    DBG_INSTR_REF !123, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !456
 
 Instruction numbers are directly attached to machine instructions with an
 optional ``debug-instr-number`` attachment, before the optional
 ``debug-location`` attachment. The value defined in ``$rbp`` in the code
 above would be identified by the pair ``<1, 0>``.
 
 The 3rd operand of the ``DBG_INSTR_REF`` above records the instruction
 and operand number ``<1, 0>``, identifying the value defined by the ``MOV64ri``.
 The first two operands to ``DBG_INSTR_REF`` are identical to ``DBG_VALUE_LIST``,
 and the ``DBG_INSTR_REF`` s position records where the variable takes on the
 designated value in the same way.
 
 More information about how these constructs are used is available in
 :doc:`InstrRefDebugInfo`. The related documents :doc:`SourceLevelDebugging` and
 :doc:`HowToUpdateDebugInfo` may be useful as well.

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
 
   /// Return whether this is an implicit location description.
   bool isImplicit() const;
 
   /// Return whether the location is computed on the expression stack, meaning
   /// it cannot be a simple register location.
   bool isComplex() const;
 
+  /// Return whether the evaluated expression makes use of a single location at
+  /// the start of the expression, i.e. if it contains only a single
+  /// DW_OP_LLVM_arg op as its first operand, or if it contains none.
+  bool isSingleLocationExpression() const;
+
   /// Append \p Ops with operations to apply the \p Offset.
   static void appendOffset(SmallVectorImpl<uint64_t> &Ops, int64_t Offset);
 
   /// If this is a constant offset, extract it. If there is no expression,
   /// return true with an offset of zero.
   bool extractIfOffset(int64_t &Offset) const;
 
   /// Returns true iff this DIExpression contains at least one instance of

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (auto *SP = dyn_cast<DISubprogram>(V->getScope())) {
     StringRef Name = SP->getName();
     if (!Name.empty())
       OS << Name << ":";
   }
   OS << V->getName();
   OS << " <- ";
 
   const DIExpression *Expr = MI->getDebugExpression();
+  if (Expr->getNumElements() && Expr->isSingleLocationExpression() &&
+      Expr->expr_op_begin()->getOp() == dwarf::DW_OP_LLVM_arg) {
+    SmallVector<uint64_t> Ops(
+        make_range(Expr->elements_begin() + 2, Expr->elements_end()));
+    Expr = DIExpression::get(Expr->getContext(), Ops);
+  }
   if (Expr->getNumElements()) {
     OS << '[';
     ListSeparator LS;
-    for (auto Op : Expr->expr_ops()) {
+    for (auto &Op : Expr->expr_ops()) {
       OS << LS << dwarf::OperationEncodingString(Op.getOp());
       for (unsigned I = 0; I < Op.getNumArgs(); ++I)
         OS << ' ' << Op.getArg(I);
     }
     OS << "] ";
   }
 
   // Register or immediate value. Register 0 means undef.

llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h

class DbgValueLoc {
 
   SmallVector<DbgValueLocEntry, 2> ValueLocEntries;
 
   bool IsVariadic;
 
 public:
   DbgValueLoc(const DIExpression *Expr, ArrayRef<DbgValueLocEntry> Locs)
       : Expression(Expr), ValueLocEntries(Locs.begin(), Locs.end()),
-        IsVariadic(true) {
-#ifndef NDEBUG
-    // Currently, DBG_VALUE_VAR expressions must use stack_value.
-    assert(Expr && Expr->isValid() &&
-           is_contained(Locs, dwarf::DW_OP_stack_value));
-#endif
-  }
+        IsVariadic(true) {}
 
   DbgValueLoc(const DIExpression *Expr, ArrayRef<DbgValueLocEntry> Locs,
               bool IsVariadic)
       : Expression(Expr), ValueLocEntries(Locs.begin(), Locs.end()),
         IsVariadic(IsVariadic) {
 #ifndef NDEBUG
     assert(cast<DIExpression>(Expr)->isValid() ||
            !any_of(Locs, [](auto LE) { return LE.isLocation(); }));
     if (!IsVariadic) {
       assert(ValueLocEntries.size() == 1);
-    } else {
-      // Currently, DBG_VALUE_VAR expressions must use stack_value.
-      assert(Expr && Expr->isValid() &&
-             is_contained(Expr->getElements(), dwarf::DW_OP_stack_value));
     }
 #endif
   }
 
   DbgValueLoc(const DIExpression *Expr, DbgValueLocEntry Loc)
       : Expression(Expr), ValueLocEntries(1, Loc), IsVariadic(false) {
     assert(((Expr && Expr->isValid()) || !Loc.isLocation()) &&
            "DBG_VALUE with a machine location must have a valid expression.");

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

if (LocID >= NumRegs) {
         bool StackValue = false;
 
         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 && !Properties.IsVariadic) {
+        } 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.
           OffsetOps.push_back(dwarf::DW_OP_deref_size);
           OffsetOps.push_back(DerefSizeInBytes);
           StackValue = true;
-        } else if (Expr->isComplex()) {
+        } 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 {
           // A plain value that has been spilt to the stack, with no further
           // context. Request a location expression, marking the DBG_VALUE as
           // IsIndirect.
           Indirect = true;

llvm/lib/CodeGen/LiveDebugVariables.cpp

if (LocNoVec.size() < 64) {
       }
     } else {
       LLVM_DEBUG(dbgs() << "Found debug value with 64+ unique machine "
                            "locations, dropping...\n");
       LocNoCount = 1;
       // Turn this into an undef debug value list; right now, the simplest form
       // of this is an expression with one arg, and an undef debug operand.
       Expression =
-          DIExpression::get(Expr.getContext(), {dwarf::DW_OP_LLVM_arg, 0,
-                                                dwarf::DW_OP_stack_value});
+          DIExpression::get(Expr.getContext(), {dwarf::DW_OP_LLVM_arg, 0});
       if (auto FragmentInfoOpt = Expr.getFragmentInfo())
         Expression = *DIExpression::createFragmentExpression(
             Expression, FragmentInfoOpt->OffsetInBits,
             FragmentInfoOpt->SizeInBits);
       LocNos = std::make_unique<unsigned[]>(LocNoCount);
       LocNos[0] = UndefLocNo;
     }
   }

llvm/lib/IR/DebugInfoMetadata.cpp

if (getNumElements() == 0)
     return false;
 
   // If there are any elements other than fragment or tag_offset, then some
   // kind of complex computation occurs.
   for (const auto &It : expr_ops()) {
     switch (It.getOp()) {
     case dwarf::DW_OP_LLVM_tag_offset:
     case dwarf::DW_OP_LLVM_fragment:
+    case dwarf::DW_OP_LLVM_arg:
       continue;
     default:
       return true;
     }
   }
 
   return false;
 }
 
+bool DIExpression::isSingleLocationExpression() const {
+  if (!isValid())
+    return false;
+
+  if (getNumElements() == 0)
+    return true;
+
+  auto ExprOpBegin = expr_ops().begin();
+  auto ExprOpEnd = expr_ops().end();
+  if (ExprOpBegin->getOp() == dwarf::DW_OP_LLVM_arg)
+    ++ExprOpBegin;
+
+  return !std::any_of(ExprOpBegin, ExprOpEnd, [](auto Op) {
+    return Op.getOp() == dwarf::DW_OP_LLVM_arg;
+  });
+}
+
 Optional<DIExpression::FragmentInfo>
 DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {
   for (auto I = Start; I != End; ++I)
     if (I->getOp() == dwarf::DW_OP_LLVM_fragment) {
       DIExpression::FragmentInfo Info = {I->getArg(1), I->getArg(0)};
       return Info;
     }
   return None;
if (!any_of(Expr->expr_ops(),
     assert(ArgNo == 0 &&
            "Location Index must be 0 for a non-variadic expression.");
     SmallVector<uint64_t, 8> NewOps(Ops.begin(), Ops.end());
     return DIExpression::prependOpcodes(Expr, NewOps, StackValue);
   }
 
   SmallVector<uint64_t, 8> NewOps;
   for (auto Op : Expr->expr_ops()) {
+    // A DW_OP_stack_value comes at the end, but before a DW_OP_LLVM_fragment.
+    if (StackValue) {
+      if (Op.getOp() == dwarf::DW_OP_stack_value)
+        StackValue = false;
+      else if (Op.getOp() == dwarf::DW_OP_LLVM_fragment) {
+        NewOps.push_back(dwarf::DW_OP_stack_value);
+        StackValue = false;
+      }
+    }
     Op.appendToVector(NewOps);
     if (Op.getOp() == dwarf::DW_OP_LLVM_arg && Op.getArg(0) == ArgNo)
       NewOps.insert(NewOps.end(), Ops.begin(), Ops.end());
   }
+  if (StackValue)
+    NewOps.push_back(dwarf::DW_OP_stack_value);
 
   return DIExpression::get(Expr->getContext(), NewOps);
 }
 
 DIExpression *DIExpression::replaceArg(const DIExpression *Expr,
                                        uint64_t OldArg, uint64_t NewArg) {
   assert(Expr && "Can't replace args in this expression");
 

llvm/lib/Transforms/Utils/Local.cpp

for (auto *DII : DbgUsers) {
     bool IsValidSalvageExpr = SalvagedExpr->getNumElements() <= MaxExpressionSize;
     if (AdditionalValues.empty() && IsValidSalvageExpr) {
       DII->setExpression(SalvagedExpr);
     } else if (isa<DbgValueInst>(DII) && IsValidSalvageExpr &&
                DII->getNumVariableLocationOps() + AdditionalValues.size() <=
                    MaxDebugArgs) {
       DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);
     } else {
-      // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is
-      // currently only valid for stack value expressions.
+      // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is not
+      // currently supported in those instructions.
       // Also do not salvage if the resulting DIArgList would contain an
       // unreasonably large number of values.
       Value *Undef = UndefValue::get(I.getOperand(0)->getType());
       DII->replaceVariableLocationOp(I.getOperand(0), Undef);
     }
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
     Salvaged = true;
   }