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

39

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

39

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

 class DwarfExpression {
 protected:
   /// Holds information about all subregisters comprising a register location.
   struct Register {
     int DwarfRegNo;
     unsigned SubRegSize;
     const char *Comment;
 
-    /// Create a full register, not extra DW_OP_piece operators necessary.
+    /// 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};

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

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(Register::createSubRegister(
             -1, Offset - CurPos, "no DWARF register encoding"));
-      if (Offset == 0 && Size == MaxSize)
+      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;
   }

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 ("p1")
+# 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")
 
-# CHECK-NOT: DW_AT_location
-# CHECK: DW_TAG
 --- |
   target triple = "thumbv7s-apple-ios"
-  define hidden void @f() !dbg !118 {
+  define hidden void @f() !dbg !5 {
   for.body:
-  ret void, !dbg !260
+  ret void, !dbg !20
   }
   !llvm.module.flags = !{!1, !2}
-  !llvm.dbg.cu = !{!6}
+  !llvm.dbg.cu = !{!3}
   !1 = !{i32 7, !"Dwarf Version", i32 4}
   !2 = !{i32 2, !"Debug Info Version", i32 3}
-  !6 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !50, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
-  !50 = !DIFile(filename: "t.cpp", directory: "/")
-  !118 = distinct !DISubprogram(name: "f",scope: !50, file: !50, line: 1, type: !120, scopeLine: 1, unit: !6)
-  !120 = !DISubroutineType(types: !{})
-  !134 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "uint8x8x2_t", file: !50, line: 113, size: 128, flags: DIFlagTypePassByValue, elements: !{})
-  !139 = !DILocalVariable(name: "p1", scope: !118, file: !50, line: 1, type: !134)
-  !260 = !DILocation(line: 0, scope: !118)
+  !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 !260
-    DBG_VALUE $q8, 0, !139, !DIExpression(DW_OP_plus_uconst, 208, DW_OP_LLVM_fragment, 0, 64), debug-location !260
+    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