Fix the remainder of PR22762 (GDB is crashing on DW_OP_piece being used inside of DW_AT_frame_base)

[aprantl]

Summary

http://llvm.org/bugs/show_bug.cgi?id=22762
The symptom was that DW_AT_frame_base should never use a DW_OP_(bit)_piece, the bug was that AddMachineRegPiece incorrectly created pieces to describe values that occupy only a subregister. Change this to emit a bit mask instead.

I'm posting this for review because emitting the bit mask increases the size occupied for DWARF expressions for sub-registers (~5 bytes for a 32-bit subregister). Previously we would (incorrectly!) use DW_OP_piece to describe a value occupying part of a register. However, "DW_OP_piece provides a way of describing how large a part of a variable a particular DWARF location description refers to.", not the size and offset of an entire variable inside a super-register. The way that most debuggers implement DW_OP_piece this sort of works out for subregisters that are at offset 0, but it causes confusion if the expression needs to be composed (such as in DW_AT_frame_base, or if the subregister contains only a part of the variable).

[aprantl]

For completeness, here is the updated version that uses DW_OP_piece where it is safe to do so (for subregisters at offset 0). In the end it doesn't actually look all that ugly.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1575 ↗

(On Diff #21746)

This code looks repetitious (same as the code in addAddress) - should it be wrapped up somewhere?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

81

Maybe name this flag "MayUsePiece" - I imagine that if GDB has trouble with piece for subregs, it'd have trouble with it for anything else too?

102

Not quite sure what this asciiart is meant to add?

115

I'd phrase this the other way - this comment should just describe the obvious case in the positive "use the shorter encoding if no shifting is required" (paraphrased - those exact words are a bit too brief)

121

This looks like it should be an elseif - since the last line of the previous if block establishes a condition that can't possibly be true for this condition anyway, right?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

550 ↗

(On Diff #21746)

Same repetition again

[aprantl]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1575 ↗

(On Diff #21746)

Yes absolutely, I intentionally wrote it that way so I could factor out the (now) obvious similarities in an NFC follow-up commit.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

81

I'm pretty sure we would have heard that by now. Besides, if this was a general flag to disable DW_OP_piece, what condition should it be triggered on: a command line option?

102

Good point, renaming Size -> SubRegSize seems to carry the same information.

115

"Use the shorter DW_OP_piece to describe a subregister at offset 0."

121

Correct.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

550 ↗

(On Diff #21746)

I'd like to clean this up in a follow-up commit since this is orthogonal to this bugfix.

[aprantl]

Address remaining comments (sans the refactoring).

[hjl.tools]

Maybe it is specific to x32. When a register is used as frame register,
the upper 32 bits are guaranteed to zero by hardware. We don't need
DW_OP_piece to describe the lower 32 bits of a frame register.

[aprantl]

[Just noticed that I forgot to rename MayUsePieceForSubRegMask in DwarfCompileUnit.]

The problem is that the TargetRegisterInfo tells us that the frame register is ebp, for which there is no separate register number, so it must be constructed from rbp. The DWARF backend doesn't know about what is in the upper half of rbp so it must mask out the upper bits.

[hjl.tools]

In D8233#140148, @aprantl wrote:

[Just noticed that I forgot to rename MayUsePieceForSubRegMask in DwarfCompileUnit.]

The problem is that the TargetRegisterInfo tells us that the frame register is ebp, for which there is no separate register number, so it must be constructed from rbp. The DWARF backend doesn't know about what is in the upper half of rbp so it must mask out the upper bits.

Not if ebp is used as frame register. It is OK to use rbp's DWARF register
number to map ebp when ebp is used as frame register.

[aprantl]

I think I'm ok with generally disabling DW_OP_piece inside of DW_AT_frame_base, but I would like to avoid putting in a target-specific hack (emitting rbp instead of ebp inside of DW_AT_frame_base) to work around a bug in GDB.

lib/CodeGen/AsmPrinter/DwarfExpression.h

public:
   void AddReg(int DwarfReg, const char *Comment = nullptr);
   /// Emit an (double-)indirect dwarf register operation.
   void AddRegIndirect(int DwarfReg, int Offset, bool Deref = false);
 
   /// Emit a dwarf register operation for describing
   /// - a small value occupying only part of a register or
   /// - a register representing only part of a value.
   void AddOpPiece(unsigned SizeInBits, unsigned OffsetInBits = 0);
-  /// Emit a shift-left dwarf expression.
-  void AddShl(unsigned ShiftBy);
-  /// Emit a shift-right dwarf expression.
-  void AddShr(unsigned ShiftBy);
 
   /// Emit an indirect dwarf register operation for the given machine register.
   /// \return false if no DWARF register exists for MachineReg.
   bool AddMachineRegIndirect(unsigned MachineReg, int Offset = 0);
 
   /// \brief Emit a partial DWARF register operation.
   /// \param MachineReg        the register
   /// \param PieceSizeInBits   size and

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

if (OffsetInBits > 0 || SizeInBits % SizeOfByte) {
     EmitUnsigned(OffsetInBits);
   } else {
     EmitOp(dwarf::DW_OP_piece);
     unsigned ByteSize = SizeInBits / SizeOfByte;
     EmitUnsigned(ByteSize);
   }
 }
 
-void DwarfExpression::AddShl(unsigned ShiftBy) {
-  EmitOp(dwarf::DW_OP_constu);
-  EmitUnsigned(ShiftBy);
-  EmitOp(dwarf::DW_OP_shl);
-}
-
-void DwarfExpression::AddShr(unsigned ShiftBy) {
-  EmitOp(dwarf::DW_OP_constu);
-  EmitUnsigned(ShiftBy);
-  EmitOp(dwarf::DW_OP_shr);
-}
-
 bool DwarfExpression::AddMachineRegIndirect(unsigned MachineReg, int Offset) {
   if (isFrameRegister(MachineReg)) {
     // If variable offset is based in frame register then use fbreg.
     EmitOp(dwarf::DW_OP_fbreg);
     EmitSigned(Offset);
     return true;
   }
 
   int DwarfReg = TRI.getDwarfRegNum(MachineReg, false);
   if (DwarfReg < 0)
     return false;
 
   AddRegIndirect(DwarfReg, Offset);
   return true;
 }
 
 bool DwarfExpression::AddMachineRegPiece(unsigned MachineReg,
                                          unsigned PieceSizeInBits,
                                          unsigned PieceOffsetInBits) {
   if (!TRI.isPhysicalRegister(MachineReg))
     return false;
 
   int Reg = TRI.getDwarfRegNum(MachineReg, false);
 
   // If this is a valid register number, emit it.
   if (Reg >= 0) {
     AddReg(Reg);
     if (PieceSizeInBits)
       AddOpPiece(PieceSizeInBits, PieceOffsetInBits);
     return true;
   }
 
   // Walk up the super-register chain until we find a valid number.
   // For example, EAX on x86_64 is a 32-bit piece 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 SubRegOffset = TRI.getSubRegIdxOffset(Idx);
-      unsigned SuperSize = TRI.getRegClass(*SR)->getSize() * 8;
-      assert(SuperSize >= (SubRegOffset + Size));
+      uint64_t Size = TRI.getSubRegIdxSize(Idx);
+      uint64_t SubRegOffset = TRI.getSubRegIdxOffset(Idx);
+      // +--------------+------+-----------+
+      // | SubRegOffset | Size | UpperBits |
+      // +--------------+------+-----------+
       AddReg(Reg, "super-register");
-      // Mask out the uninteresting higher bits by shifting left first.
-      unsigned UpperBits = SuperSize - (SubRegOffset+Size);
-      if (UpperBits)
-        AddShl(UpperBits);
-      // If this is part of a variable in a sub-register at a
-      // non-zero offset, we need to manually shift the value into
-      // place, since the DW_OP_piece describes the part of the
-      // variable, not the position of the subregister.
-      if (SubRegOffset+UpperBits)
-        AddShr(SubRegOffset+UpperBits);
+      // If this is in a sub-register at a non-zero offset, we need to manually
+      // shift the value into place, since the DW_OP_piece describes the part of
+      // the variable, not the position of the subregister.
+      if (SubRegOffset > 0) {
+        EmitOp(dwarf::DW_OP_constu);
+        EmitUnsigned(SubRegOffset);
+        EmitOp(dwarf::DW_OP_shr);
+      }
+      if (Size > PieceSizeInBits) {
+        // Mask out the uninteresting higher bits.
+        // Use a DW_OP_and with a constant for this, because the size of DWARF
+        // stack elements is not well defined.
+        assert(Size <= 64 && "cannot create a mask larger than 64 bits");
+        uint64_t Mask = ~uint64_t(0ULL) >> (64 - Size);
+        EmitOp(dwarf::DW_OP_constu);
+        EmitUnsigned(Mask);
+        EmitOp(dwarf::DW_OP_and);
+      }
       if (PieceSizeInBits)
-        AddOpPiece(Size, PieceOffsetInBits);
+        AddOpPiece(PieceSizeInBits, PieceOffsetInBits);
       return true;
     }
   }
 
   // Otherwise, attempt to find a covering set of sub-register numbers.
   // For example, Q0 on ARM is a composition of D0+D1.
   //
   // Keep track of the current position so we can emit the more

test/CodeGen/ARM/debug-info-s16-reg.ll

 ; RUN: llc < %s - | FileCheck %s
 ; Radar 9309221
 ; Test dwarf reg no for s16
 ;CHECK: super-register DW_OP_regx
 ;CHECK-NEXT: 264
-;CHECK-NEXT: DW_OP_piece
-;CHECK-NEXT: 4
+;CHECK-NEXT: DW_OP_constu
+;CHECK-NEXT: 4294967295
+;CHECK-NEXT: DW_OP_and
 
 target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:32-f32:32:32-f64:32:32-v64:32:64-v128:32:128-a0:0:32-n32"
 target triple = "thumbv7-apple-macosx10.6.7"
 
 @.str = private unnamed_addr constant [11 x i8] c"%p %lf %c\0A\00"
 @.str1 = private unnamed_addr constant [6 x i8] c"point\00"
 
 define i32 @inlineprinter(i8* %ptr, float %val, i8 zeroext %c) nounwind optsize ssp {

test/CodeGen/ARM/debug-info-sreg2.ll

 ; RUN: llc < %s - -filetype=obj | llvm-dwarfdump -debug-dump=loc - | FileCheck %s
 ; Radar 9376013
 target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:32-f32:32:32-f64:32:32-v64:32:64-v128:32:128-a0:0:32-n32"
 target triple = "thumbv7-apple-macosx10.6.7"
 
 ; Just making sure the first part of the location isn't a repetition
 ; of the size of the location description.
 ;
 ; 0x90   DW_OP_regx of super-register
-
 ; CHECK: 0x00000000: Beginning address offset:
 ; CHECK-NEXT:           Ending address offset:
-; CHECK-NEXT:            Location description: 90 {{.. .. .. .. $}}
+; d8, constu 0xffffffff, and
+; CHECK-NEXT:            Location description: 90 88 02 10 ff ff ff ff 0f 1a
 
 define void @_Z3foov() optsize ssp {
 entry:
   %call = tail call float @_Z3barv() optsize, !dbg !11
   tail call void @llvm.dbg.value(metadata float %call, i64 0, metadata !5, metadata !MDExpression()), !dbg !11
   %call16 = tail call float @_Z2f2v() optsize, !dbg !12
   %cmp7 = fcmp olt float %call, %call16, !dbg !12
   br i1 %cmp7, label %for.body, label %for.end, !dbg !12

test/DebugInfo/ARM/s-super-register.ll

 ; RUN: llc < %s - -filetype=obj | llvm-dwarfdump -debug-dump=loc - | FileCheck %s
 target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:32-f32:32:32-f64:32:32-v64:32:64-v128:32:128-a0:0:32-n32"
 target triple = "thumbv7-apple-macosx10.6.7"
 
 ; The S registers on ARM are expressed as pieces of their super-registers in DWARF.
 ;
 ; 0x90   DW_OP_regx of super-register
-; 0x93   DW_OP_piece
-; 0x9d   DW_OP_bit_piece
-; CHECK:            Location description: 90 {{.. .. ((93 ..)|(9d .. ..)) $}}
+;                                           d8, constu 0xffffffff, and 
+; CHECK:            Location description: 90 88 02 10 ff ff ff ff 0f 1a 
 
 define void @_Z3foov() optsize ssp {
 entry:
   %call = tail call float @_Z3barv() optsize, !dbg !11
   tail call void @llvm.dbg.value(metadata float %call, i64 0, metadata !5, metadata !MDExpression()), !dbg !11
   %call16 = tail call float @_Z2f2v() optsize, !dbg !12
   %cmp7 = fcmp olt float %call, %call16, !dbg !12
   br i1 %cmp7, label %for.body, label %for.end, !dbg !12

test/DebugInfo/X86/dbg-value-const-byref.ll

 ;        consts 0x00000003
 ; CHECK: Beginning address offset: 0x0000000000000{{.*}}
 ; CHECK:    Ending address offset: [[C1:.*]]
 ; CHECK:     Location description: 11 03
 ;        consts 0x00000007
 ; CHECK: Beginning address offset: [[C1]]
 ; CHECK:    Ending address offset: [[C2:.*]]
 ; CHECK:     Location description: 11 07
-;        rax, piece 0x00000004
+;        rax & 0xffffffff
 ; CHECK: Beginning address offset: [[C2]]
 ; CHECK:    Ending address offset: [[R1:.*]]
-; CHECK:     Location description: 50 93 04
+; CHECK:     Location description: 50 10 ff ff ff ff 0f 1a 
 ;         rdi+0
 ; CHECK: Beginning address offset: [[R1]]
 ; CHECK:    Ending address offset: [[R2:.*]]
 ; CHECK:     Location description: 75 00
 ;
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 

test/DebugInfo/X86/fission-ranges.ll

 ; if they've changed due to a bugfix, change in register allocation, etc.
 
 ; CHECK: [[A]]: Beginning address index: 2
 ; CHECK-NEXT:                    Length: 190
 ; CHECK-NEXT:      Location description: 11 00
 ; CHECK-NEXT: {{^$}}
 ; CHECK-NEXT:   Beginning address index: 3
 ; CHECK-NEXT:                    Length: 23
-; CHECK-NEXT:      Location description: 50 93 04
+; CHECK-NEXT:      Location description: 50 10 ff ff ff ff 0f 1a
 ; CHECK: [[E]]: Beginning address index: 4
 ; CHECK-NEXT:                    Length: 21
-; CHECK-NEXT:      Location description: 50 93 04
+; CHECK-NEXT:      Location description: 50 10 ff ff ff ff 0f 1a
 ; CHECK: [[B]]: Beginning address index: 5
 ; CHECK-NEXT:                    Length: 19
-; CHECK-NEXT:      Location description: 50 93 04
+; CHECK-NEXT:      Location description: 50 10 ff ff ff ff 0f 1a
 ; CHECK: [[D]]: Beginning address index: 6
 ; CHECK-NEXT:                    Length: 23
-; CHECK-NEXT:      Location description: 50 93 04
+; CHECK-NEXT:      Location description: 50 10 ff ff ff ff 0f 1a
 
 ; Make sure we don't produce any relocations in any .dwo section (though in particular, debug_info.dwo)
 ; HDR-NOT: .rela.{{.*}}.dwo
 
 ; Make sure we have enough stuff in the debug_addr to cover the address indexes
 ; (6 is the last index in debug_loc.dwo, making 7 entries of 8 bytes each, 7 * 8
 ; == 56 base 10 == 38 base 16)
 

test/DebugInfo/X86/subreg.ll

 ; RUN: llc %s -mtriple=x86_64-pc-linux-gnu -O0 -o - | FileCheck %s
 
 ; We are testing that a value in a 16 bit register gets reported as
 ; being in its superregister.
 
 ; CHECK: .byte   80                      # super-register DW_OP_reg0
 ; CHECK-NEXT: .byte   147                # DW_OP_piece
-; CHECK-NEXT: .byte   2                  # 2
+; CHECK-NEXT: DW_OP_constu
+; CHECK-NEXT: 4294967295
+; CHECK-NEXT: DW_OP_and
 
 define i16 @f(i16 signext %zzz) nounwind {
 entry:
   call void @llvm.dbg.value(metadata i16 %zzz, i64 0, metadata !0, metadata !MDExpression())
   %conv = sext i16 %zzz to i32, !dbg !7
   %conv1 = trunc i32 %conv to i16
   ret i16 %conv1
 }