Fix an assertion failure in DwarfExpression's subregister composition

[aprantl]

Summary

This patch fixes an assertion failure in DwarfExpression that is triggered when a complex fragment has exactly
the size of a subregister of the register the DBG_VALUE points to *and* there is no DWARF encoding for the super-register.

I took the opportunity to replace/document some magic values with static constructor functions to make this code less confusing to read.

rdar://problem/58489125

[vsk]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

154

Is CurSubReg the intersection between the bits already emitted & the bits covered by the subreg, or just the bits covered by this subreg?

163

So, here: DwarfRegs.push_back(Register::createSubRegister(-1, Offset - CurPos, "no DWARF register encoding")

Are we saying: bits [0, Offset) of the value aren't going to be described?

165

This (DwarfRegs.push_back(Register::createRegister(Reg, "sub-register")) is the only functional change, right? IIUC this means that we're describing bits [0, MaxSize) of the value. Can we break out of the MCSubRegIterator loop here?

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

113

s/not/no/

llvm/test/DebugInfo/MIR/ARM/subregister-full-piece.mir

40

Why is the DW_OP_plus_uconst needed?

[bjope]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

164

If you add a second test case with for example DW_OP_LLVM_fragment, 0, 56, then I guess that you still will hit the assert (you'll go into the else below due to Size>MaxSize and then you'll end up with a single sub-register. Well, I haven't tested it myself so I might be wrong.

So maybe the condition should be Size>=MaxSize. But then we end up without adding any DW_OP_piece/DW_OP_bit_piece, so we'd rely on that the DW_OP_piece is implicit if the described type is smaller than the register location. A caveat with that might be that afaik it is ABI specific which bits in the register that should be used when describing a DW_OP_piece using a register location that is larger in size (so is that true also for an implicit piece?).

On the other hand, aren't there some other troubles here (not neccessarily to be solved by this patch). We can't just generally adjust the size of the register to read based on the fragment size, right? Specially considering the addition of typed info for the DW ops.

Assume we got:

DBG_VALUE $q8, 0, !139, !DIExpression(DW_OP_shr, 32, DW_OP_LLVM_fragment, 0, 64)

It would be easy to assume that it means, read q8, shift right 32 steps, and then use the 64 least significant bits to describe the 64 bit fragment of !139. If we do DW_OP_bregx D16 we won't shift down bits from D17. Or is it clear that we only take n-bits from the register when having an n-bit fragment?

Another example could be:

DBG_VALUE $reg16bit, 0, !139, !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 8, ,DW_ATE_signed, DW_OP_stack_value, DW_OP_LLVM_fragment, 8, 8)

Here we got a 8-bit fragment being described using a 16-bit register. Not sure if we generate this kind of expressions, but my point is that the number of referenced bits on the left of the expression does not neccessarily match the size of the fragment on the right of the expression nowadays. That might be a problem.

165

You'd still need to update CurPos if bailing out. But we could probably bail out from the loop, in general, after the CurPos increment when CurPos >= MaxSize.

[aprantl]

llvm/test/DebugInfo/MIR/ARM/subregister-full-piece.mir

40

This isn't strictly necessary. It was there in the testcase I reduced this from, and I thought it was needed to go down the HasComplexExpression() path, but the test also works without it.

[aprantl]

Address review feedback.

Bjorn made an excellent point about operations that need access to more bits, such as DW_OP_shr. I will address that in a follow-up soon!

[djtodoro]

@aprantl Thanks for this! I think something went wrong with the latest patch you updated, I guess you posted a wrong one. :)

It might be reasonable to split this into an NFC and functional change, since we will try to address the example @bjope described as well.

[aprantl]

Updated with the full patch again. Sorry!

[aprantl]

Ping.

[djtodoro]

LGTM.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

 /// buffer, which can later on be committed to the main output. The size of the
 /// temporary buffer is queryable, allowing for the size of the data to be
 /// emitted before the data is committed.
 class DwarfExpression {
 protected:
   /// Holds information about all subregisters comprising a register location.
   struct Register {
     int DwarfRegNo;
-    unsigned Size;
+    unsigned SubRegSize;
     const char *Comment;
+
+    /// Create a full register, no extra DW_OP_piece operators necessary.
+    static Register createRegister(int RegNo, const char *Comment) {
+      return {RegNo, 0, Comment};
+    }
+
+    /// Create a subregister that needs a DW_OP_piece operator with SizeInBits.
+    static Register createSubRegister(int RegNo, unsigned SizeInBits,
+                                      const char *Comment) {
+      return {RegNo, SizeInBits, Comment};
+    }
+
+    bool isSubRegister() const { return SubRegSize; }
   };
 
   /// Whether we are currently emitting an entry value operation.
   bool IsEmittingEntryValue = false;
 
   DwarfCompileUnit &CU;
 
   /// The register location, if any.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

void DwarfExpression::addAnd(unsigned Mask) {
   emitConstu(Mask);
   emitOp(dwarf::DW_OP_and);
 }
 
 bool DwarfExpression::addMachineReg(const TargetRegisterInfo &TRI,
                                     unsigned MachineReg, unsigned MaxSize) {
   if (!llvm::Register::isPhysicalRegister(MachineReg)) {
     if (isFrameRegister(TRI, MachineReg)) {
-      DwarfRegs.push_back({-1, 0, nullptr});
+      DwarfRegs.push_back(Register::createRegister(-1, nullptr));
       return true;
     }
     return false;
   }
 
   int Reg = TRI.getDwarfRegNum(MachineReg, false);
 
   // If this is a valid register number, emit it.
   if (Reg >= 0) {
-    DwarfRegs.push_back({Reg, 0, nullptr});
+    DwarfRegs.push_back(Register::createRegister(Reg, nullptr));
     return true;
   }
 
   // Walk up the super-register chain until we find a valid number.
   // For example, EAX on x86_64 is a 32-bit fragment of RAX with offset 0.
   for (MCSuperRegIterator SR(MachineReg, &TRI); SR.isValid(); ++SR) {
     Reg = TRI.getDwarfRegNum(*SR, false);
     if (Reg >= 0) {
       unsigned Idx = TRI.getSubRegIndex(*SR, MachineReg);
       unsigned Size = TRI.getSubRegIdxSize(Idx);
       unsigned RegOffset = TRI.getSubRegIdxOffset(Idx);
-      DwarfRegs.push_back({Reg, 0, "super-register"});
+      DwarfRegs.push_back(Register::createRegister(Reg, "super-register"));
       // Use a DW_OP_bit_piece to describe the sub-register.
       setSubRegisterPiece(Size, RegOffset);
       return true;
     }
   }
 
   // Otherwise, attempt to find a covering set of sub-register numbers.
   // For example, Q0 on ARM is a composition of D0+D1.
bool DwarfExpression::addMachineReg(const TargetRegisterInfo &TRI,
   for (MCSubRegIterator SR(MachineReg, &TRI); SR.isValid(); ++SR) {
     unsigned Idx = TRI.getSubRegIndex(MachineReg, *SR);
     unsigned Size = TRI.getSubRegIdxSize(Idx);
     unsigned Offset = TRI.getSubRegIdxOffset(Idx);
     Reg = TRI.getDwarfRegNum(*SR, false);
     if (Reg < 0)
       continue;
 
-    // Intersection between the bits we already emitted and the bits
-    // covered by this subregister.
+    // Used to build the intersection between the bits we already
+    // emitted and the bits covered by this subregister.
     SmallBitVector CurSubReg(RegSize, false);
     CurSubReg.set(Offset, Offset + Size);
 
     // If this sub-register has a DWARF number and we haven't covered
     // its range, and its range covers the value, emit a DWARF piece for it.
     if (Offset < MaxSize && CurSubReg.test(Coverage)) {
       // Emit a piece for any gap in the coverage.
       if (Offset > CurPos)
-        DwarfRegs.push_back(
-            {-1, Offset - CurPos, "no DWARF register encoding"});
-      DwarfRegs.push_back(
-          {Reg, std::min<unsigned>(Size, MaxSize - Offset), "sub-register"});
+        DwarfRegs.push_back(Register::createSubRegister(
+            -1, Offset - CurPos, "no DWARF register encoding"));
+      if (Offset == 0 && Size >= MaxSize)
+        DwarfRegs.push_back(Register::createRegister(Reg, "sub-register"));
+      else
+        DwarfRegs.push_back(Register::createSubRegister(
+            Reg, std::min<unsigned>(Size, MaxSize - Offset), "sub-register"));
     }
     // Mark it as emitted.
     Coverage.set(Offset, Offset + Size);
     CurPos = Offset + Size;
   }
   // Failed to find any DWARF encoding.
   if (CurPos == 0)
     return false;
   // Found a partial or complete DWARF encoding.
   if (CurPos < RegSize)
-    DwarfRegs.push_back({-1, RegSize - CurPos, "no DWARF register encoding"});
+    DwarfRegs.push_back(Register::createSubRegister(
+        -1, RegSize - CurPos, "no DWARF register encoding"));
   return true;
 }
 
 void DwarfExpression::addStackValue() {
   if (DwarfVersion >= 4)
     emitOp(dwarf::DW_OP_stack_value);
 }
 
bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
   // a call site parameter expression and if that expression is just a register
   // location, emit it with addBReg and offset 0, because we should emit a DWARF
   // expression representing a value, rather than a location.
   if (!isMemoryLocation() && !HasComplexExpression && (!isParameterValue() ||
                                                        isEntryValue())) {
     for (auto &Reg : DwarfRegs) {
       if (Reg.DwarfRegNo >= 0)
         addReg(Reg.DwarfRegNo, Reg.Comment);
-      addOpPiece(Reg.Size);
+      addOpPiece(Reg.SubRegSize);
     }
 
     if (isEntryValue())
       finalizeEntryValue();
 
     if (isEntryValue() && !isParameterValue() && DwarfVersion >= 4)
       emitOp(dwarf::DW_OP_stack_value);
 
if (any_of(ExprCursor, [](DIExpression::ExprOperand Op) -> bool {
       LocationKind = Unknown;
       return false;
     }
 
   assert(DwarfRegs.size() == 1);
   auto Reg = DwarfRegs[0];
   bool FBReg = isFrameRegister(TRI, MachineReg);
   int SignedOffset = 0;
-  assert(Reg.Size == 0 && "subregister has same size as superregister");
+  assert(!Reg.isSubRegister() && "full register expected");
 
   // Pattern-match combinations for which more efficient representations exist.
   // [Reg, DW_OP_plus_uconst, Offset] --> [DW_OP_breg, Offset].
   if (Op && (Op->getOp() == dwarf::DW_OP_plus_uconst)) {
     uint64_t Offset = Op->getArg(0);
     uint64_t IntMax = static_cast<uint64_t>(std::numeric_limits<int>::max());
     if (Offset <= IntMax) {
       SignedOffset = Offset;

llvm/test/DebugInfo/MIR/ARM/subregister-full-piece.mir

+# RUN: llc -start-after=livedebugvalues -filetype=obj -o - %s | \
+# RUN:    llvm-dwarfdump - | FileCheck %s
+#
+# This tests the edge-case where a complex fragment has exactly
+# the size of a subregister of the register the DBG_VALUE points to.
+#
+# CHECK: .debug_info contents:
+# CHECK: DW_TAG_variable
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_location
+#                                     Q8 = {D16, D17}
+# CHECK-NEXT:                   DW_OP_bregx D16+208, DW_OP_piece 0x8)
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_name ("q8")
+# CHECK: DW_TAG_variable
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_location
+#                                     Q9 = {D18, D19}
+# CHECK-NEXT:                   DW_OP_bregx D18+0, DW_OP_piece 0x7)
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_name ("q9")
+
+--- |
+  target triple = "thumbv7s-apple-ios"
+  define hidden void @f() !dbg !5 {
+  for.body:
+  ret void, !dbg !20
+  }
+  !llvm.module.flags = !{!1, !2}
+  !llvm.dbg.cu = !{!3}
+  !1 = !{i32 7, !"Dwarf Version", i32 4}
+  !2 = !{i32 2, !"Debug Info Version", i32 3}
+  !3 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !4, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+  !4 = !DIFile(filename: "t.cpp", directory: "/")
+  !5 = distinct !DISubprogram(name: "f",scope: !4, file: !4, line: 1, type: !6, scopeLine: 1, unit: !3)
+  !6 = !DISubroutineType(types: !{})
+  !7 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "uint8x8x2_t", file: !4, line: 113, size: 128, flags: DIFlagTypePassByValue, elements: !{})
+  !8 = !DILocalVariable(name: "q8", scope: !5, file: !4, line: 1, type: !7)
+  !9 = !DILocalVariable(name: "q9", scope: !5, file: !4, line: 1, type: !7)
+  !10 = !DILocalVariable(name: "q10", scope: !5, file: !4, line: 1, type: !7)
+  !20 = !DILocation(line: 0, scope: !5)
+name:            f
+body:             |
+  bb.2.for.body:
+    t2Bcc %bb.2.for.body, 0, killed $cpsr, debug-location !20
+    DBG_VALUE $q8, 0, !8, !DIExpression(DW_OP_plus_uconst, 208, DW_OP_LLVM_fragment, 0, 64), debug-location !20
+    DBG_VALUE $q9, 0, !9, !DIExpression(DW_OP_LLVM_fragment, 0, 56), debug-location !20
+    tB %bb.2.for.body, 14, $noreg