[DwarfDebug] Refuse to emit DW_OP_LLVM_arg values wider than 64 bits

[rickyz]

Summary

DwarfExpression::addUnsignedConstant(const APInt &Value) only supports
wider-than-64-bit values when it is used to emit a top-level DWARF
expression representing the location of a variable. Before this change,
it was possible to call addUnsignedConstant on >64 bit values within a
subexpression when substituting DW_OP_LLVM_arg values.

This can trigger an assertion failure (e.g. PR52584, PR52333) when it
happens in a fragment (DW_OP_LLVM_fragment) expression, as
addUnsignedConstant on >64 bit values splits the constant into separate
DW_OP_pieces, which modifies DwarfExpression::OffsetInBits.

This change papers over the assertion errors by bailing on overly wide
DW_OP_LLVM_arg values. A more comprehensive fix might be to be to split
wide values into pointer-sized fragments.

[0] https://github.com/llvm/llvm-project/blob/e71fa03/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#L799-L805

[rickyz]

Minor rewording/test cleanup.

[rickyz]

Update commit message.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

806

Can you add a comment explaining what the more principled fix would be to the code you added?

llvm/test/DebugInfo/X86/pr52584.ll

2

Can you FileCheck for something (ideally the missing DW_AT_location) here? Otherwise this test even succeeds if you symlink llc to /bin/true.

[Mordante]

Thanks for working on this!

I just tested this patch with D70631, the review where I discovered the issue.
After I applied your patch the build issue I had has been resolved.

[rickyz]

Respond to comments, avoid emitting a partially-filled DW_AT_location on encountering an overly wide argument.

[rickyz]

Minor comment wording change.

[aprantl]

Thanks!

[rickyz]

Thanks for the review! I don't have commit access, so can you please commit this on my behalf? Thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (auto *DVal = DV.getValueLoc()) {
     assert(Expr && "Variadic Debug Value must have an Expression.");
     DIELoc *Loc = new (DIEValueAllocator) DIELoc;
     DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
     DwarfExpr.addFragmentOffset(Expr);
     DIExpressionCursor Cursor(Expr);
     const TargetRegisterInfo &TRI = *Asm->MF->getSubtarget().getRegisterInfo();
 
     auto AddEntry = [&](const DbgValueLocEntry &Entry,
-                                            DIExpressionCursor &Cursor) {
       if (Entry.isLocation()) {
         if (!DwarfExpr.addMachineRegExpression(TRI, Cursor,
                                                Entry.getLoc().getReg()))
           return false;
       } else if (Entry.isInt()) {
         // If there is an expression, emit raw unsigned bytes.
         DwarfExpr.addUnsignedConstant(Entry.getInt());
       } else if (Entry.isConstantFP()) {
+        // DwarfExpression does not support arguments wider than 64 bits
+        // (see PR52584).
+        // TODO: Consider chunking expressions containing overly wide
+        // arguments into separate pointer-sized fragment expressions.
         APInt RawBytes = Entry.getConstantFP()->getValueAPF().bitcastToAPInt();
-        DwarfExpr.addUnsignedConstant(RawBytes);
+        if (RawBytes.getBitWidth() > 64)
+          return false;
+        DwarfExpr.addUnsignedConstant(RawBytes.getZExtValue());
       } else if (Entry.isConstantInt()) {
         APInt RawBytes = Entry.getConstantInt()->getValue();
-        DwarfExpr.addUnsignedConstant(RawBytes);
+        if (RawBytes.getBitWidth() > 64)
+          return false;
+        DwarfExpr.addUnsignedConstant(RawBytes.getZExtValue());
       } else if (Entry.isTargetIndexLocation()) {
         TargetIndexLocation Loc = Entry.getTargetIndexLocation();
         // TODO TargetIndexLocation is a target-independent. Currently only the
         // WebAssembly-specific encoding is supported.
         assert(Asm->TM.getTargetTriple().isWasm());
         DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
       } else {
         llvm_unreachable("Unsupported Entry type.");
       }
       return true;
     };
 
-    DwarfExpr.addExpression(
+    if (!DwarfExpr.addExpression(
             std::move(Cursor),
             [&](unsigned Idx, DIExpressionCursor &Cursor) -> bool {
               return AddEntry(DVal->getLocEntries()[Idx], Cursor);
-        });
+            }))
+      return VariableDie;
 
     // Now attach the location information to the DIE.
     addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
     if (DwarfExpr.TagOffset)
       addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset, dwarf::DW_FORM_data1,
               *DwarfExpr.TagOffset);
 
     return VariableDie;

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

public:
   /// which needs to be calculated at time of emission, so we don't emit
   /// any operands here.
   void beginEntryValueExpression(DIExpressionCursor &ExprCursor);
 
   /// Return the index of a base type with the given properties and
   /// create one if necessary.
   unsigned getOrCreateBaseType(unsigned BitSize, dwarf::TypeKind Encoding);
 
+  /// Emit all remaining operations in the DIExpressionCursor. The
+  /// cursor must not contain any DW_OP_LLVM_arg operations.
+  void addExpression(DIExpressionCursor &&Expr);
+
   /// Emit all remaining operations in the DIExpressionCursor.
-  ///
-  /// \param FragmentOffsetInBits     If this is one fragment out of multiple
-  ///                                 locations, this is the offset of the
-  ///                                 fragment inside the entire variable.
-  void addExpression(DIExpressionCursor &&Expr,
-                     unsigned FragmentOffsetInBits = 0);
-  void
-  addExpression(DIExpressionCursor &&Expr,
-                llvm::function_ref<bool(unsigned, DIExpressionCursor &)> InsertArg);
+  /// DW_OP_LLVM_arg operations are resolved by calling (\p InsertArg).
+  //
+  /// \return false if any call to (\p InsertArg) returns false.
+  bool addExpression(
+      DIExpressionCursor &&Expr,
 
   /// If applicable, emit an empty DW_OP_piece / DW_OP_bit_piece to advance to
   /// the fragment described by \c Expr.
   void addFragmentOffset(const DIExpression *Expr);
 
   void emitLegacySExt(unsigned FromBits);
   void emitLegacyZExt(unsigned FromBits);
 

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

case dwarf::DW_OP_LLVM_fragment:
       break;
     default:
       return false;
     }
   }
   return true;
 }
 
-void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
-                                    unsigned FragmentOffsetInBits) {
+void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor) {
   addExpression(std::move(ExprCursor),
                 [](unsigned Idx, DIExpressionCursor &Cursor) -> bool {
                   llvm_unreachable("unhandled opcode found in expression");
                 });
 }
 
-void DwarfExpression::addExpression(
+bool DwarfExpression::addExpression(
     DIExpressionCursor &&ExprCursor,
     llvm::function_ref<bool(unsigned, DIExpressionCursor &)> InsertArg) {
   // Entry values can currently only cover the initial register location,
   // and not any other parts of the following DWARF expression.
   assert(!IsEmittingEntryValue && "Can't emit entry value around expression");
 
   Optional<DIExpression::ExprOperand> PrevConvertOp = None;
 
   while (ExprCursor) {
     auto Op = ExprCursor.take();
     uint64_t OpNum = Op->getOp();
 
     if (OpNum >= dwarf::DW_OP_reg0 && OpNum <= dwarf::DW_OP_reg31) {
       emitOp(OpNum);
       continue;
     } else if (OpNum >= dwarf::DW_OP_breg0 && OpNum <= dwarf::DW_OP_breg31) {
       addBReg(OpNum - dwarf::DW_OP_breg0, Op->getArg(0));
       continue;
     }
 
     switch (OpNum) {
     case dwarf::DW_OP_LLVM_arg:
       if (!InsertArg(Op->getArg(0), ExprCursor)) {
         LocationKind = Unknown;
-        return;
+        return false;
       }
       break;
     case dwarf::DW_OP_LLVM_fragment: {
       unsigned SizeInBits = Op->getArg(1);
       unsigned FragmentOffset = Op->getArg(0);
       // The fragment offset must have already been adjusted by emitting an
       // empty DW_OP_piece / DW_OP_bit_piece before we emitted the base
       // location.
case dwarf::DW_OP_LLVM_fragment: {
       if (isImplicitLocation())
         addStackValue();
 
       // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
       setSubRegisterPiece(0, 0);
       // Reset the location description kind.
       LocationKind = Unknown;
-      return;
+      return true;
     }
     case dwarf::DW_OP_plus_uconst:
       assert(!isRegisterLocation());
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
while (ExprCursor) {
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
 
   if (isImplicitLocation() && !isParameterValue())
     // Turn this into an implicit location description.
     addStackValue();
+
+  return true;
 }
 
 /// add masking operations to stencil out a subregister.
 void DwarfExpression::maskSubRegister() {
   assert(SubRegisterSizeInBits && "no subregister was registered");
   if (SubRegisterOffsetInBits > 0)
     addShr(SubRegisterOffsetInBits);
   uint64_t Mask = (1ULL << (uint64_t)SubRegisterSizeInBits) - 1ULL;

llvm/test/DebugInfo/X86/pr52584.ll

+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -filetype=obj -o - %s \
+; RUN:     | llvm-dwarfdump - \
+; RUN:     | FileCheck %s
+
+; CHECK:      DW_TAG_variable
+; CHECK-NOT:    DW_AT_location
+; CHECK-NEXT:   DW_AT_name  ("arr")
+; CHECK-NOT:    DW_AT_location
+; CHECK:      DW_TAG
+define dso_local void @test() !dbg !4 {
+entry:
+  call void @llvm.dbg.value(metadata !DIArgList(i128 0), metadata !7, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 127)), !dbg !12
+  ret void, !dbg !12
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 1, type: !5, unit: !0)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !DILocalVariable(name: "arr", scope: !4, file: !1, line: 1, type: !8)
+!8 = !DICompositeType(tag: DW_TAG_array_type, baseType: !9, size: 128, elements: !10)
+!9 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!10 = !{!11}
+!11 = !DISubrange(count: 16)
+!12 = !DILocation(line: 1, column: 1, scope: !4)