[DWARF] Fix a few corner cases in expression emission

[loladiro]

Summary

I noticed an object file with DW_OP_reg4 DW_OP_breg4 0 as a DWARF expression, which I traced to a missing break (and ++I) in this code snippet. While I was at it, I also added support for a few other corner cases along the same lines that I could think of. Please have a look - I've read the DWARF spec, but am I by no means an expert.

[aprantl]

Thanks for looking into this! I have a few questions inline.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1565 ↗

(On Diff #27277)

Hopefully this can become an assertion with Duncan's pending patch.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

219 ↗

(On Diff #27277)

This comment now needs to be moved down to the "if (DW_OP_deref)" block.

237 ↗

(On Diff #27277)

The indirection here seems wrong:
To reach this clause, we need an expression like "DW_OP_reg Offset DW_OP_plus (DW_OP_bit_Piece x y)?"
Shouldn't this be just an unconditional AddMachineRegPiece()?

[loladiro]

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

219 ↗

(On Diff #27277)

Will move.

237 ↗

(On Diff #27277)

We could also be in the case that N == E. I think the right thing to do is actually to put the AddOpStackValue(); here, AddExpression below will add the DW_OP_piece if there is one.

[loladiro]

Address review comments.

[aprantl]

We could also be in the case that N == E. I think the right thing to do is actually to put the AddOpStackValue(); here, AddExpression below will add the DW_OP_piece if there is one.

That's what I meant with the regex-style question at the end of "DW_OP_reg Offset DW_OP_plus (DW_OP_bit_Piece x y)?".
My point was that in either case, the DW_OP_plus is the last arithmetic operation and that in the absence of a DW_OP_deref we should not be emitting an (=breg) register-indirect value.

The IR-expression

[machinereg no. 5] 4 DW_OP_plus

should translate into

DW_OP_reg DW_OP_constu 5 DW_OP_plus

and not into

DW_OP_breg 5 DW_OP_constu 5 DW_OP_plus

[loladiro]

Is DW_OP_reg5 DW_OP_plus_uconst 4 actually valid DWARF? My reading of the spec was that you had to express that as
DW_OP_breg5 4 DW_OP_stack_value, but of course I can't argue with a DWARF committee member ;).

[aprantl]

Oh you definitely should, especially if they are giving sloppy examples!

What I should have written is: The IR

[machinereg no. 5] 4 DW_OP_plus

should become (in DWARF v5):

DW_OP_regval_type 5 [type ofs] DW_OP_constu 4 DW_OP_plus DW_OP_stack_value

whereas the IR

[machinereg no. 5] 4 DW_OP_plus DW_OP_deref

should become (in any DWARF version):

DW_OP_breg 5

You are absolutely correct that the first expression isn't actually expressible in DWARF 4 and before.

[loladiro]

Ok, just for clarity, why wouldn't DW_OP_breg5 4 DW_OP_stack_value work for the first case?

[aprantl]

I think you're right. The definition of DW_OP_stack_value is vague but the examples speak a clear language:

DW_OP_breg5 4

is equivalent to

DW_OP_breg5 4 DW_OP_deref DW_OP_stack_value

and means (in C notation)

*(reg5+4)

And

DW_OP_breg5 4 DW_OP_stack_value

indeed means

reg5+4

[aprantl]

This convinced me that we really need a much better separation between the DWARF 4 and pre-DWARF 4 expression generation. No need to address this in this patch, but this needs to be fixed.

Thanks!

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::emitDebugLoc() {
   // Start the dwarf loc section.
   Asm->OutStreamer->SwitchSection(
       Asm->getObjFileLowering().getDwarfLocSection());
   unsigned char Size = Asm->getDataLayout().getPointerSize();
   for (const auto &List : DebugLocs.getLists()) {
     Asm->OutStreamer->EmitLabel(List.Label);
     const DwarfCompileUnit *CU = List.CU;
     for (const auto &Entry : DebugLocs.getEntries(List)) {
+       if (Entry.BeginSym == Entry.EndSym)
+        continue;
       // Set up the range. This range is relative to the entry point of the
       // compile unit. This is a hard coded 0 for low_pc when we're emitting
       // ranges, or the DW_AT_low_pc on the compile unit otherwise.
       if (auto *Base = CU->getBaseAddress()) {
         Asm->EmitLabelDifference(Entry.BeginSym, Base, Size);
         Asm->EmitLabelDifference(Entry.EndSym, Base, Size);
       } else {
         Asm->OutStreamer->EmitSymbolValue(Entry.BeginSym, Size);

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.h

public:
   /// function will attempt to emit a DWARF register by emitting a
   /// piece of a super-register or by piecing together multiple
   /// subregisters that alias the register.
   ///
   /// \return false if no DWARF register exists for MachineReg.
   bool AddMachineRegPiece(unsigned MachineReg, unsigned PieceSizeInBits = 0,
                           unsigned PieceOffsetInBits = 0);
 
+  /// Emit a DW_OP_stack_value
+  void AddOpStackValue();
+
   /// Emit a signed constant.
   void AddSignedConstant(int Value);
   /// Emit an unsigned constant.
   void AddUnsignedConstant(unsigned Value);
 
   /// \brief Emit an entire expression on top of a machine register location.
   ///
   /// \param PieceOffsetInBits If this is one piece out of a fragmented

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

 }
 
 void DwarfExpression::AddShr(unsigned ShiftBy) {
   EmitOp(dwarf::DW_OP_constu);
   EmitUnsigned(ShiftBy);
   EmitOp(dwarf::DW_OP_shr);
 }
 
+void DwarfExpression::AddOpStackValue() {
+  if (DwarfVersion >= 4)
+    EmitOp(dwarf::DW_OP_stack_value);
+}
+
 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;
   }
 
void DwarfExpression::AddSignedConstant(int Value) {
   // Unfortunately, DW_OP_stack_value was not available until DWARF-4,
   // so we will continue to generate DW_OP_constu <const> for DWARF-2
   // and DWARF-3. Technically, this is incorrect since DW_OP_const <const>
   // actually describes a value at a constant addess, not a constant value.
   // However, in the past there was no better way  to describe a constant
   // value, so the producers and consumers started to rely on heuristics
   // to disambiguate the value vs. location status of the expression.
   // See PR21176 for more details.
-  if (DwarfVersion >= 4)
-    EmitOp(dwarf::DW_OP_stack_value);
+  AddOpStackValue();
 }
 
 void DwarfExpression::AddUnsignedConstant(unsigned Value) {
   EmitOp(dwarf::DW_OP_constu);
   EmitUnsigned(Value);
   // cf. comment in DwarfExpression::AddSignedConstant().
-  if (DwarfVersion >= 4)
-    EmitOp(dwarf::DW_OP_stack_value);
+  AddOpStackValue();
 }
 
 static unsigned getOffsetOrZero(unsigned OffsetInBits,
                                 unsigned PieceOffsetInBits) {
   if (OffsetInBits == PieceOffsetInBits)
     return 0;
   assert(OffsetInBits >= PieceOffsetInBits && "overlapping pieces");
   return OffsetInBits;
bool DwarfExpression::AddMachineRegExpression(const DIExpression *Expr,
   case dwarf::DW_OP_bit_piece: {
     unsigned OffsetInBits = I->getArg(0);
     unsigned SizeInBits   = I->getArg(1);
     // Piece always comes at the end of the expression.
     return AddMachineRegPiece(MachineReg, SizeInBits,
                getOffsetOrZero(OffsetInBits, PieceOffsetInBits));
   }
   case dwarf::DW_OP_plus: {
-    // [DW_OP_reg,Offset,DW_OP_plus,DW_OP_deref] --> [DW_OP_breg,Offset].
     auto N = I.getNext();
-    if (N != E && N->getOp() == dwarf::DW_OP_deref) {
-      unsigned Offset = I->getArg(0);
+    // First combine all DW_OP_plus until we hit either a DW_OP_deref or a
+    // DW_OP_bit_piece
+    while (N != E && N->getOp() == dwarf::DW_OP_plus) {
+      Offset += N->getArg(0);
+      ++I;
+      N = I.getNext();
+    }
+    if (N != E && N->getOp() == dwarf::DW_OP_deref) {
+      // [DW_OP_reg,Offset,DW_OP_plus,DW_OP_deref] --> [DW_OP_breg,Offset].
       ValidReg = AddMachineRegIndirect(MachineReg, Offset);
       std::advance(I, 2);
-      break;
-    } else
+    } else {
+      assert ((N == E) || (N->getOp() == dwarf::DW_OP_bit_piece));
+      if (Offset == 0) {
         ValidReg = AddMachineRegPiece(MachineReg);
+      } else {
+        ValidReg = AddMachineRegIndirect(MachineReg, Offset);
+        AddOpStackValue();
+      }
+      ++I;
+    }
+    break;
   }
   case dwarf::DW_OP_deref: {
       // [DW_OP_reg,DW_OP_deref] --> [DW_OP_breg].
       ValidReg = AddMachineRegIndirect(MachineReg);
       ++I;
       break;
   }
   default:
     llvm_unreachable("unsupported operand");
   }
 
   if (!ValidReg)
     return false;
 
   // Emit remaining elements of the expression.
   AddExpression(I, E, PieceOffsetInBits);
+
   return true;
 }
 
 void DwarfExpression::AddExpression(DIExpression::expr_op_iterator I,
                                     DIExpression::expr_op_iterator E,
                                     unsigned PieceOffsetInBits) {
   for (; I != E; ++I) {
     switch (I->getOp()) {

llvm/trunk/test/DebugInfo/expressions.ll

+; REQUIRES: object-emission
+; RUN: %llc_dwarf -mtriple x86_64-apple-darwin14.0.0-elf -filetype=obj %s -o %t
+; RUN: %llc_dwarf -mtriple x86_64-apple-darwin14.0.0-elf -O0 -filetype=obj %s -o %t0
+; RUN: llvm-dwarfdump -debug-dump=loc %t | FileCheck %s
+; RUN: llvm-dwarfdump -debug-dump=loc %t0 | FileCheck -check-prefix CHECK-O0 %s
+
+; CHECK: 0x00000000: Beginning address offset: 0x0000000000000000
+; CHECK:                Ending address offset: 0x[[END:[0-9a-f]+]]
+; CHECK:                 Location description:
+; CHECK-NOT:                                   75 00 55
+; CHECK-SAME:                                  55
+; CHECK: 0x00000023: Beginning address offset: 0x0000000000000000
+; CHECK:                Ending address offset: 0x{{.*}}[[END]]
+; CHECK:                 Location description: 75 08 9f
+; CHECK: 0x00000048: Beginning address offset: 0x0000000000000000
+; CHECK:                Ending address offset: 0x{{.*}}[[END]]
+; CHECK:                 Location description: 75 10 9f
+; CHECK: 0x0000006d: Beginning address offset: 0x0000000000000000
+; CHECK:                Ending address offset: 0x{{.*}}[[END]]
+; CHECK:                 Location description: 75 18
+
+
+; CHECK-O0: 0x00000000: Beginning address offset: 0x0000000000000000
+; CHECK-O0:                Ending address offset: 0x000000000000001b
+; CHECK-O0:                 Location description: 55
+; CHECK-O0:             Beginning address offset: 0x000000000000001b
+; CHECK-O0:                Ending address offset: 0x0000000000000024
+; CHECK-O0:                 Location description: 54
+; CHECK-O0:             Beginning address offset: 0x0000000000000024
+; CHECK-O0:                Ending address offset: 0x0000000000000025
+; CHECK-O0:                 Location description: 77 78 23 00
+; CHECK-O0: 0x0000004c: Beginning address offset: 0x0000000000000000
+; CHECK-O0:                Ending address offset: 0x000000000000001b
+; CHECK-O0:                 Location description: 75 08 9f
+; CHECK-O0:             Beginning address offset: 0x000000000000001b
+; CHECK-O0:                Ending address offset: 0x0000000000000024
+; CHECK-O0:                 Location description: 74 08 9f
+; CHECK-O0:             Beginning address offset: 0x0000000000000024
+; CHECK-O0:                Ending address offset: 0x0000000000000025
+; CHECK-O0:                 Location description: 77 78 23 08
+; CHECK-O0: 0x0000009c: Beginning address offset: 0x0000000000000000
+; CHECK-O0:                Ending address offset: 0x000000000000001b
+; CHECK-O0:                 Location description: 75 10 9f
+; CHECK-O0:             Beginning address offset: 0x000000000000001b
+; CHECK-O0:                Ending address offset: 0x0000000000000024
+; CHECK-O0:                 Location description: 74 10 9f
+; CHECK-O0:             Beginning address offset: 0x0000000000000024
+; CHECK-O0:                Ending address offset: 0x0000000000000025
+; CHECK-O0:                 Location description: 77 78 23 08 23 08
+; CHECK-O0: 0x000000ee: Beginning address offset: 0x0000000000000000
+; CHECK-O0:                Ending address offset: 0x000000000000001b
+; CHECK-O0:                 Location description: 75 18
+; CHECK-O0:             Beginning address offset: 0x000000000000001b
+; CHECK-O0:                Ending address offset: 0x0000000000000024
+; CHECK-O0:                 Location description: 74 18
+; CHECK-O0:             Beginning address offset: 0x0000000000000024
+; CHECK-O0:                Ending address offset: 0x0000000000000025
+; CHECK-O0:                 Location description: 77 78 23 10 23 08 06
+
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #0
+
+define float @foo(float* %args, float *%args2)
+{
+    call void @llvm.dbg.value(metadata float* %args, i64 0, metadata !11, metadata !12), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args, i64 0, metadata !13, metadata !14), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args, i64 0, metadata !15, metadata !16), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args, i64 0, metadata !17, metadata !18), !dbg !19
+    %a = load float, float* %args, !dbg !19
+    %bptr = getelementptr float, float* %args, i32 1, !dbg !19
+    %b = load float, float* %bptr, !dbg !19
+    %cptr = getelementptr float, float* %args, i32 2, !dbg !19
+    %c = load float, float* %cptr, !dbg !19
+    %dptr = getelementptr float, float* %args, i32 3, !dbg !19
+    %d = load float, float* %dptr, !dbg !19
+    %ret1 = fadd float %a, %b, !dbg !19
+    %ret2 = fadd float %c, %d, !dbg !19
+    call void @llvm.dbg.value(metadata float* %args2, i64 0, metadata !11, metadata !12), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args2, i64 0, metadata !13, metadata !14), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args2, i64 0, metadata !15, metadata !16), !dbg !19
+    call void @llvm.dbg.value(metadata float* %args2, i64 0, metadata !17, metadata !18), !dbg !19
+    %ret  = fsub float %ret1, %ret2, !dbg !19
+    ret float %ret, !dbg !19
+}
+
+attributes #0 = { nounwind readnone }
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+
+!0 = !{i32 2, !"Dwarf Version", i32 4}
+!1 = !{i32 1, !"Debug Info Version", i32 3}
+
+!2 = !DICompileUnit(language: DW_LANG_C89, file: !3, producer: "byHand", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !4, retainedTypes: !4, subprograms: !5, globals: !4, imports: !4)
+!3 = !DIFile(filename: "expressions", directory: ".")
+!4 = !{}
+!5 = !{!6}
+!6 = !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !3, type: !7, isLocal: false, isDefinition: true, isOptimized: true, function: float (float*, float*)* @foo, variables: !4)
+!7 = !DISubroutineType(types: !8)
+!8 = !{!10, !10}
+!9 = !DIBasicType(name: "float", size: 4, align: 4, encoding: DW_ATE_float)
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 64, align: 64)
+!11 = !DILocalVariable(tag: DW_TAG_arg_variable, name: "a", arg: 1, scope: !6, file: !3, line: 1, type: !10)
+!12 = !DIExpression(DW_OP_plus, 0)
+!13 = !DILocalVariable(tag: DW_TAG_arg_variable, name: "b", arg: 2, scope: !6, file: !3, line: 1, type: !10)
+!14 = !DIExpression(DW_OP_plus, 8)
+!15 = !DILocalVariable(tag: DW_TAG_arg_variable, name: "c", arg: 3, scope: !6, file: !3, line: 1, type: !10)
+!16 = !DIExpression(DW_OP_plus, 8, DW_OP_plus, 8)
+!17 = !DILocalVariable(tag: DW_TAG_arg_variable, name: "d", arg: 4, scope: !6, file: !3, line: 1, type: !9)
+!18 = !DIExpression(DW_OP_plus, 16, DW_OP_plus, 8, DW_OP_deref)
+!19 = !DILocation(line: 1, scope: !6)