Handle complex DWARF expressions in combination with "complex" registers

[aprantl]

Summary

Thanks to @bjope for pointing out this problem!

Here's my proposed solution:

If the register itself can only be described by combining multiple subregisters, we cannot use DW_OP_piece, since it doesn't safely compose with another complex expression, Since it is not possible to apply any DWARF operation to the combined DW_OP_pieces. By manualy shifting the subregisters into place, this can be worked around at the expense of a slightly larger encoding.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

296

One thing I don't like about this myself is that this is a special case of the code in addExpression().

[aprantl]

llvm/test/DebugInfo/MIR/ARM/split-superreg-complex.mir

1

I'd appreciate someone double-checking my reasoning here; but I do think that in the case of a DBG_VALUE tied to a single register, which doesn't have a DWARF number of its own, this is okay.

[aemerson]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

225

I think you can use llvm::scope_exit for this kind of thing?

[aprantl]

Use llvm::scope_exit as suggested by Amara. Thanks, I was looking for something like that!

[dstenb]

llvm/test/DebugInfo/MIR/ARM/subregister-complex-expression.mir

12

I don't have much knowledge about AArch64 so I might be overlooking something here, but I wonder if this expression really is valid, given the size of an element on the DWARF expression stack. The size of the elements on the stack is the size of an address on the target machine, which I assume is 64 bits on AArch64? DWARFv4 (and DWARFv5 for the generic type) specifies that the result of DW_OP_shl is truncated to fit in an element (modulo one plus the largest representable address), so it seems that the result of (D17 << 64) should be 0, and we'll just end up with the value of (D16 >> 32)?

Is this a case where we need to use DWARFv5's type operations (e.g. using DW_OP_regval_type to push the registers) to allow for the operations to be performed on a 128-bit base type?

I think there in general are quite a lot of cases where LLVM needs to consider the size of the elements on the expression stack in which it currently doesn't.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

241

Nit: s/manualy/manually/

246

Nit: s/in are/in the registers are/

250

Shouldn't there be a version check so that we don't emit DW_OP_regval_type when targeting earlier DWARF standards?

if (Large && DwarfVersion < 5)
  return false;

(Should we perhaps consider using the GNU extension, DW_OP_GNU_regval_type, for earlier versions?)

406

Can this change be moved to a preceding NFC commit to reduce this patch's size?

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

250

(Should we perhaps consider using the GNU extension, DW_OP_GNU_regval_type, for earlier versions?)

+1
I don't see any obstacle for that. We can extend the DwarfCompileUnit::getDwarf5OrGNULocationAtom() supporting the location atom as well.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

260–261

The DWARFv5 standard specifies that all operands of logical and arithmetic operations must have the same type, so I think that the shift amount operand must use the same base type as the registers here.

The same issue also exists for the complex expressions that are applied to this function's result, for example the DW_OP_shr in the attached test case.

There already exists similar issues for DW_OP_LLVM_convert. I recently discovered a case for our downstream target where the following was emitted:

DW_OP_bregx 0xf0 +0, DW_OP_convert (0x0000017b) "DW_ATE_unsigned_32", DW_OP_convert (0x00000191) "DW_ATE_unsigned_16", DW_OP_lit31, DW_OP_and, DW_OP_stack_value

The DW_OP_and is done with one unsigned 16-bit base type operand, and one operand of the generic type. GDB throws an Incompatible types on DWARF stack error when trying to print that variable. I'll see if I can create a downstream reproducer for that and write a Bugzilla ticket.

I guess DwarfExpression in some way must keep track of the element types.

[bjope]

I'd prefer if you didn't mix refactoring and functional changes (assuming that the changes to the DwarfExpression::Register struct could be done as an NFC regardless of what happens to this patch). Currently I think this is both a little bit hard to review, and it will become a mess to integrate downstream (even if that is hard for you to predict).

We got a couple of special cases that we currently need to handle downstream in addMachineRegExpression involving types that aren't a multiple of the byte size (we need to pad with extra pieces to satisfy the debugger). And for some reason we also need to reverse the order of all pieces since they should describe the variable in increasing memory order, and at least for our big-endian target (together with the iteration order of MCSubRegIterator) we get things in the wrong order unless we reverse the DwarfReg vector at the end of addMachineReg. Well, this is hopefully nothing that you need to care about, but right now it is hard for me to see how things would work when this patch has landed. And temporarily maintaining two implemenations would also be hard if the framework is refactored/renamed as part of the patch.

Sorry if I misunderstood something (if the DwarfExpression::Register changes wouldn't make sense at all as a standalone patch).

[aprantl]

Factored out the NFC bits. Have not yet addressed @dstenb 's comment yet.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

bool DwarfExpression::addMachineReg(const TargetRegisterInfo &TRI,
 
   // Otherwise, attempt to find a covering set of sub-register numbers.
   // For example, Q0 on ARM is a composition of D0+D1.
   unsigned CurPos = 0;
   // The size of the register in bits.
   const TargetRegisterClass *RC = TRI.getMinimalPhysRegClass(MachineReg);
   unsigned RegSize = TRI.getRegSizeInBits(*RC);
   // Keep track of the bits in the register we already emitted, so we
-  // can avoid emitting redundant aliasing subregs. Because this is
-  // just doing a greedy scan of all subregisters, it is possible that
-  // this doesn't find a combination of subregisters that fully cover
-  // the register (even though one may exist).
+  // can avoid emitting redundant aliasing subregs.
   SmallBitVector Coverage(RegSize, false);
   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;
while (Offset < Size) {
     addOpPiece(std::min(Size - Offset, 64u), Offset);
     Offset += 64;
   }
 }
 
 bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
                                               DIExpressionCursor &ExprCursor,
                                               unsigned MachineReg,
                                               unsigned FragmentOffsetInBits) {
   auto ClearDwarfRegs = make_scope_exit([this] { DwarfRegs.clear(); });
 
   if (!addMachineReg(TRI, MachineReg)) {
     LocationKind = Unknown;
     return false;
   }
 
   bool HasComplexExpression = false;
   auto Op = ExprCursor.peek();
   if (Op && Op->getOp() != dwarf::DW_OP_LLVM_fragment)
     HasComplexExpression = true;
 
   // If the register itself can only be described by combining
   // multiple subregisters, we cannot use DW_OP_piece, since it
   // doesn't safely compose with another complex expression, Since it
   // is not possible to apply any DWARF operation to the combined
-  // DW_OP_pieces.
-  if (HasComplexExpression && DwarfRegs.size() > 1)
+  // DW_OP_pieces. By manualy shifting the subregisters into place,
+  // this can be worked around at the expense of a slightly larger
+  // encoding.
+  if (HasComplexExpression && DwarfRegs.size() > 1) {
+    if (isMemoryLocation())
       return false;
+    // Shift each piece into place and or the results together. No
+    // masking is necessary even if the subregisters are overlapping,
+    // since any overlapping bits in are expected be identical.
+    const TargetRegisterClass *RC = TRI.getMinimalPhysRegClass(MachineReg);
+    unsigned RegSize = TRI.getRegSizeInBits(*RC);
+    bool Large = RegSize > CU.getAddressSize();
+    bool First = true;
+    for (auto &Reg : DwarfRegs) {
+      if (Large) {
+        emitOp(dwarf::DW_OP_regval_type);
+        emitUnsigned(Reg.DwarfRegNo);
+        emitBaseTypeRef(getOrCreateBaseType(RegSize, dwarf::DW_ATE_unsigned));
+      } else {
+        addBReg(Reg.DwarfRegNo, 0, Reg.getComment());
+      }
+      if (Reg.SubRegisterOffsetInBits > 0) {
+        emitConstu(Reg.SubRegisterOffsetInBits);
+        emitOp(dwarf::DW_OP_shl);
+      }
+      if (!First)
+        emitOp(dwarf::DW_OP_or);
+      else
+        First = false;
+    }
+    return true;
+  }
 
   // Handle simple register locations. If we are supposed to emit
   // 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())) {
     unsigned BitsEmitted = 0;
     auto Fragment = ExprCursor.getFragmentInfo();
     unsigned FragmentBits =
         Fragment ? Fragment->SizeInBits : std::numeric_limits<unsigned>::max();
     for (auto &Reg : DwarfRegs) {
       switch (Reg.Kind) {
       case Register::Full:
       case Register::SuperReg:
         if (Reg.DwarfRegNo >= 0)
           addReg(Reg.DwarfRegNo, Reg.getComment());
         break;
       case Register::SubReg: {
         // If the last subregister(s) extend beyond the size of the
         // fragment, truncate the DW_OP_piece accordingly. This is safe
         // because we have a simple expression.
         unsigned SubRegBits = Reg.SubRegisterSizeInBits;
         if (BitsEmitted + SubRegBits > FragmentBits)
           SubRegBits -= (BitsEmitted + SubRegBits) - FragmentBits;
         BitsEmitted += SubRegBits;
         if (SubRegBits == 0)
           break;
         if (Reg.DwarfRegNo >= 0)
           addReg(Reg.DwarfRegNo, Reg.getComment());
         if (SubRegBits < FragmentBits)
void DwarfExpression::finalizeEntryValue() {
   emitUnsigned(Size);
 
   // Emit the entry value's DWARF block operand.
   commitTemporaryBuffer();
 
   IsEmittingEntryValue = false;
 }
 
 unsigned DwarfExpression::getOrCreateBaseType(unsigned BitSize,
                                               dwarf::TypeKind Encoding) {
   // Reuse the base_type if we already have one in this CU otherwise we
   // create a new one.
   unsigned I = 0, E = CU.ExprRefedBaseTypes.size();
   for (; I != E; ++I)
     if (CU.ExprRefedBaseTypes[I].BitSize == BitSize &&
         CU.ExprRefedBaseTypes[I].Encoding == Encoding)
       break;

llvm/test/DebugInfo/MIR/ARM/split-superreg-complex.mir

-# RUN: llc -start-before=livedebugvalues -filetype=obj -o - %s | \
-# RUN:    llvm-dwarfdump - | FileCheck %s
-#
-# This is an artificial example of a debug value residing in a composite
-# location with a complex expression. Because the semantics of applying a DWARF
-# expression to a composite location are ill-defined, the compiler should bail
-# out of emitting a location.
-#
-# CHECK: .debug_info contents:
-# CHECK: DW_TAG_variable
-# CHECK-NOT: DW_AT_location
-# CHECK: DW_TAG
---- |
-  ; Generated from:
-  ; typedef float vec2 __attribute__((vector_size(16)));
-  ; vec2 v();
-  ; float f() {
-  ;   vec2 vec = v();
-  ;   return vec[0] + vec[1];
-  ; }
-
-  target datalayout = "e-m:o-p:32:32-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
-  target triple = "thumbv7s-apple-ios5.0.0"
-  
-  define float @f() local_unnamed_addr #0 !dbg !9 {
-  entry:
-    %call = tail call <4 x float> bitcast (<4 x float> (...)* @v to <4 x float> ()*)() #0, !dbg !19
-    tail call void @llvm.dbg.value(metadata <4 x float> %call, i64 0, metadata !14, metadata !20), !dbg !21
-    %vecext = extractelement <4 x float> %call, i32 0, !dbg !22
-    %vecext1 = extractelement <4 x float> %call, i32 1, !dbg !23
-    %add = fadd float %vecext, %vecext1, !dbg !24
-    ret float %add, !dbg !25
-  }
-  
-  declare arm_aapcs_vfpcc <4 x float> @v(...) local_unnamed_addr #0
-  declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #0
-  
-  attributes #0 = { nounwind readnone }
-  
-  !llvm.dbg.cu = !{!0}
-  !llvm.module.flags = !{!3, !4}
-  !llvm.ident = !{!8}
-  
-  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0 (trunk 286322) (llvm/trunk 286305)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
-  !1 = !DIFile(filename: "v.c", directory: "/")
-  !2 = !{}
-  !3 = !{i32 2, !"Dwarf Version", i32 2}
-  !4 = !{i32 2, !"Debug Info Version", i32 3}
-  !8 = !{!"clang version 4.0.0 (trunk 286322) (llvm/trunk 286305)"}
-  !9 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 3, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: true, unit: !0, retainedNodes: !13)
-  !10 = !DISubroutineType(types: !11)
-  !11 = !{!12}
-  !12 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
-  !13 = !{!14}
-  !14 = !DILocalVariable(name: "vec", scope: !9, file: !1, line: 4, type: !15)
-  !15 = !DIDerivedType(tag: DW_TAG_typedef, name: "vec2", file: !1, line: 1, baseType: !16)
-  !16 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, size: 128, flags: DIFlagVector, elements: !17)
-  !17 = !{!18}
-  !18 = !DISubrange(count: 4)
-  !19 = !DILocation(line: 4, column: 13, scope: !9)
-  !20 = !DIExpression(DW_OP_plus_uconst, 1, DW_OP_constu, 1, DW_OP_minus)
-  !21 = !DILocation(line: 4, column: 7, scope: !9)
-  !22 = !DILocation(line: 5, column: 9, scope: !9)
-  !23 = !DILocation(line: 5, column: 18, scope: !9)
-  !24 = !DILocation(line: 5, column: 16, scope: !9)
-  !25 = !DILocation(line: 5, column: 2, scope: !9)
-
-...
----
-name:            f
-alignment:       2
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-tracksRegLiveness: true
-calleeSavedRegisters: [ '$lr', '$d8', '$d9', '$d10', '$d11', '$d12', '$d13', 
-                        '$d14', '$d15', '$q4', '$q5', '$q6', '$q7', '$r4', 
-                        '$r5', '$r6', '$r7', '$r8', '$r10', '$r11', '$s16', 
-                        '$s17', '$s18', '$s19', '$s20', '$s21', '$s22', 
-                        '$s23', '$s24', '$s25', '$s26', '$s27', '$s28', 
-                        '$s29', '$s30', '$s31', '$d8_d10', '$d9_d11', '$d10_d12', 
-                        '$d11_d13', '$d12_d14', '$d13_d15', '$q4_q5', '$q5_q6', 
-                        '$q6_q7', '$q4_q5_q6_q7', '$r4_r5', '$r6_r7', '$r10_r11', 
-                        '$d8_d9_d10', '$d9_d10_d11', '$d10_d11_d12', '$d11_d12_d13', 
-                        '$d12_d13_d14', '$d13_d14_d15', '$d8_d10_d12', 
-                        '$d9_d11_d13', '$d10_d12_d14', '$d11_d13_d15', 
-                        '$d8_d10_d12_d14', '$d9_d11_d13_d15', '$d9_d10', 
-                        '$d11_d12', '$d13_d14', '$d9_d10_d11_d12', '$d11_d12_d13_d14' ]
-frameInfo:       
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       4
-  offsetAdjustment: 0
-  maxAlignment:    4
-  adjustsStack:    true
-  hasCalls:        true
-  maxCallFrameSize: 0
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-stack:           
-  - { id: 0, type: spill-slot, offset: -4, size: 4, alignment: 4, callee-saved-register: '$lr' }
-body:             |
-  bb.0.entry:
-    liveins: $lr
-  
-    early-clobber $sp = frame-setup t2STR_PRE killed undef $lr, $sp, -4, 14, _
-    frame-setup CFI_INSTRUCTION def_cfa_offset 4
-    frame-setup CFI_INSTRUCTION offset $lr, -4
-    tBL 14, _, @v, csr_ios, implicit-def dead $lr, implicit $sp, implicit-def $sp, implicit-def $r0, implicit-def $r1, implicit-def $r2, implicit-def $r3, debug-location !19
-    $d1 = VMOVDRR killed $r2, killed $r3, 14, _, implicit-def $q0, debug-location !19
-    $d0 = VMOVDRR killed $r0, killed $r1, 14, _, implicit killed $q0, implicit-def $q0, debug-location !19
-    DBG_VALUE $q0, _, !14, !20, debug-location !21
-    $s4 = VMOVS $s1, 14, _, implicit-def $d2, debug-location !24
-    $d0 = VADDfd $d0, killed $d2, 14, _, implicit killed $q0, debug-location !24
-    $r0 = VMOVRS $s0, 14, _, implicit killed $d0, debug-location !25
-    $lr, $sp = t2LDR_POST $sp, 4, 14, _, debug-location !25
-    tBX_RET 14, _, implicit $r0, debug-location !25
-
-...

llvm/test/DebugInfo/MIR/ARM/subregister-complex-expression.mir

+# RUN: llc -start-after=livedebugvalues -filetype=obj -o - %s | \
+# RUN:    llvm-dwarfdump - | FileCheck %s
+#
+# Test applying a complex expression to a complex register.
+#
+# CHECK: .debug_info contents:
+# CHECK: DW_TAG_variable
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_location
+#                          Q8 = {D16,                                                  D17}
+# CHECK-NEXT:  DW_OP_regval_type D16 ({{.*}}) "DW_ATE_unsigned_128", DW_OP_regval_type D17 ({{.*}}) "DW_ATE_unsigned_128", DW_OP_constu 0x40, DW_OP_shl, DW_OP_or, DW_OP_constu 0x20, DW_OP_shr)
+
+# CHECK-NOT: DW_TAG
+# CHECK:   DW_AT_name ("q8")
+
+--- |
+  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)
+  !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, $noreg, !8, !DIExpression(DW_OP_constu, 32, DW_OP_shr), debug-location !20
+    tB %bb.2.for.body, 14, $noreg