[DebugInfo] Omit location list entries with empty ranges

[dstenb]

Summary

This fixes PR39710. In that case we emitted a location list looking like
this:

.Ldebug_loc0:

.quad   .Lfunc_begin0-.Lfunc_begin0
.quad   .Lfunc_begin0-.Lfunc_begin0
.short  1                       # Loc expr size
.byte   85                      # DW_OP_reg5
.quad   .Lfunc_begin0-.Lfunc_begin0
.quad   .Lfunc_end0-.Lfunc_begin0
.short  1                       # Loc expr size
.byte   85                      # super-register DW_OP_reg5
.quad   0
.quad   0

As seen, the first entry's beginning and ending addresses evalute to 0,
which meant that the entry inadvertently became a "end of list" entry,
resulting in the location list ending sooner than expected.

To fix this, omit all entries with empty ranges. Location list entries
with empty ranges do not have any effect, as specified by DWARF, so we
might as well drop them:

"A location list entry (but not a base address selection or end of list
entry) whose beginning and ending addresses are equal has no effect
because the size of the range covered by such an entry is zero."

[aprantl]

Looks generally good to me.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1188 ↗

(On Diff #179033)

why not just move the condition two lines further down?

[dstenb]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1188 ↗

(On Diff #179033)

Sorry for the late response due to the holiday season!

I'm not sure I follow. Do you mean to merge this code with the DIExpr->isFragment() code at line 1198 somehow?

[aprantl]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1188 ↗

(On Diff #179033)

Yeah, this code block looks like it is repeated further down below and I was wondering if it could be simplified / deduplicated.

[dstenb]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1188 ↗

(On Diff #179033)

If it is okay for you, I think I prefer bailing out early like this at the cost of that code duplication, in order to avoid creating an unused DebugLocEntry object, and dealing with the DebugLoc.back().MergeValues(Loc) invocation in that conditional, as I think that might make the code a bit harder to understand.

[aprantl]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1188 ↗

(On Diff #179033)

sure, your call.

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

else if (std::next(I) == Ranges.end())
       EndLabel = Asm->getFunctionEnd();
     else
       EndLabel = getLabelBeforeInsn(std::next(I)->first);
     assert(EndLabel && "Forgot label after instruction ending a range!");
 
     LLVM_DEBUG(dbgs() << "DotDebugLoc: " << *Begin << "\n");
 
     auto Value = getDebugLocValue(Begin);
+
+    // Omit entries with empty ranges as they do not have any effect in DWARF.
+    if (StartLabel == EndLabel) {
+      // If this is a fragment, we must still add the value to the list of
+      // open ranges, since it may describe non-overlapping parts of the
+      // variable.
+      if (DIExpr->isFragment())
+        OpenRanges.push_back(Value);
+      LLVM_DEBUG(dbgs() << "Omitting location list entry with empty range.\n");
+      continue;
+    }
+
     DebugLocEntry Loc(StartLabel, EndLabel, Value);
     bool couldMerge = false;
 
     // If this is a fragment, it may belong to the current DebugLocEntry.
     if (DIExpr->isFragment()) {
       // Add this value to the list of open ranges.
       OpenRanges.push_back(Value);
 
if (Value.isInt()) {
     DwarfExpr.addUnsignedConstant(RawBytes);
   }
   DwarfExpr.addExpression(std::move(ExprCursor));
 }
 
 void DebugLocEntry::finalize(const AsmPrinter &AP,
                              DebugLocStream::ListBuilder &List,
                              const DIBasicType *BT) {
+  assert(Begin != End && "unexpected location list entry with empty range");
   DebugLocStream::EntryBuilder Entry(List, Begin, End);
   BufferByteStreamer Streamer = Entry.getStreamer();
   DebugLocDwarfExpression DwarfExpr(AP.getDwarfVersion(), Streamer);
   const DebugLocEntry::Value &Value = Values[0];
   if (Value.isFragment()) {
     // Emit all fragments that belong to the same variable and range.
     assert(llvm::all_of(Values, [](DebugLocEntry::Value P) {
           return P.isFragment();

llvm/trunk/test/DebugInfo/ARM/PR26163.ll

 ; RUN: llc -filetype=obj -o - < %s | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ;
-; Checks that we're creating two ranges, one that terminates immediately
-; and one that spans the rest of the function. This isn't necessarily the
-; best thing to do here (and also not necessarily correct, since the first
-; one has a bit_piece), but it is what is currently being emitted, any
-; change here needs to be intentional, so the test is very specific.
+; Checks that we're omitting the first range, as it is empty, and that we're
+; emitting one that spans the rest of the function. In this case, the first
+; range, which we omit, describes 8 bytes of the variable using DW_OP_litX,
+; whereas the second one only describes 4 bytes, so clobbering the whole 8 byte
+; fragment with the 4 bytes fragment isn't necessarily best thing to do here,
+; but it is what is currently being emitted. Any change here needs to be
+; intentional, so the test is very specific.
 ;
 ; CHECK: DW_TAG_inlined_subroutine
 ; CHECK: DW_TAG_variable
 ; CHECK:   DW_AT_location [DW_FORM_sec_offset] ({{.*}}
-; CHECK:      [0x00000004, 0x00000004): DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x8
-; CHECK:      [0x00000004, 0x00000014): DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
+; CHECK-NEXT: [0x00000004, 0x00000014): DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
 
 ; Created form the following test case (PR26163) with
 ; clang -cc1 -triple armv4t--freebsd11.0-gnueabi -emit-obj -debug-info-kind=standalone -O2 -x c test.c
 ;
 ; typedef unsigned int  size_t;
 ; struct timeval {
 ;   long long tv_sec;
 ;   int tv_usec;

llvm/trunk/test/DebugInfo/X86/PR26148.ll

 ;  a = b = c;
 ; }
 ;
 ; int main() { return 0; }
 ;
 ; This is similar to the bug in test/DebugInfo/ARM/PR26163.ll, except that there is an
 ; extra non-overlapping range first. Thus, we make sure that the backend actually looks
 ; at all expressions when determining whether to merge ranges, not just the first one.
-; AS in 26163, we expect two ranges (as opposed to one), the first one being zero sized
+; AS in 26163, we only expect one range as the first one is zero sized.
 ;
 ;
-; CHECK: [0x0000000000000004, 0x0000000000000004): DW_OP_lit3, DW_OP_piece 0x4, DW_OP_reg5 RDI, DW_OP_piece 0x2
-; CHECK: [0x0000000000000004, 0x0000000000000014): DW_OP_lit3, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_piece 0x4
+; CHECK: [0x0000000000000000, 0x000000000000000f): DW_OP_lit3, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_piece 0x4
 
 source_filename = "test/DebugInfo/X86/PR26148.ll"
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 %struct.S0 = type { i16, i32 }
 
 @a = common global %struct.S0 zeroinitializer, align 4, !dbg !0
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.value(metadata, metadata, metadata) #0
-; The attributes are here to force the zero-sized range not to be at the start of
-; the function, which has special interpretation in DWARF. The fact that this happens
-; at all is probably an LLVM bug.
 
-define void @fn1(i16 signext %p1) #1 !dbg !16 {
+define void @fn1(i16 signext %p1) !dbg !16 {
 entry:
   tail call void @llvm.dbg.value(metadata i16 %p1, metadata !20, metadata !23), !dbg !24
   tail call void @llvm.dbg.declare(metadata %struct.S0* undef, metadata !21, metadata !23), !dbg !25
   tail call void @llvm.dbg.declare(metadata %struct.S0* undef, metadata !22, metadata !23), !dbg !26
   tail call void @llvm.dbg.value(metadata i32 3, metadata !22, metadata !27), !dbg !26
   tail call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !28), !dbg !26
   tail call void @llvm.dbg.value(metadata i16 %p1, metadata !21, metadata !29), !dbg !25
   tail call void @llvm.dbg.value(metadata i32 3, metadata !21, metadata !27), !dbg !25
   tail call void @llvm.dbg.value(metadata i32 0, metadata !21, metadata !28), !dbg !25
   store i32 3, i32* bitcast (%struct.S0* @a to i32*), align 4, !dbg !30
   store i32 0, i32* getelementptr inbounds (%struct.S0, %struct.S0* @a, i64 0, i32 1), align 4, !dbg !30
   ret void, !dbg !31
 }
 
 define i32 @main() !dbg !32 {
 entry:
   ret i32 0, !dbg !35
 }
 
 attributes #0 = { nounwind readnone }
-attributes #1 = { "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" }
 
 !llvm.dbg.cu = !{!2}
 !llvm.module.flags = !{!12, !13, !14}
 !llvm.ident = !{!15}
 
 !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
 !1 = !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 4, type: !6, isLocal: false, isDefinition: true)
 !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 3.9.0 (https://github.com/llvm-mirror/clang 8f258397c5afd7a708bd95770c718e81d08fb11a) (https://github.com/llvm-mirror/llvm 18481855bdfa1b4a424f81be8525db002671348d)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)

llvm/trunk/test/DebugInfo/X86/pieces-3.ll

 ;    int foo(Outer outer) {
 ;      Inner i1 = outer.inner[1];
 ;      return i1.a;
 ;    }
 ;
 ; CHECK: DW_TAG_formal_parameter [3]
 ; CHECK-NEXT:   DW_AT_location [DW_FORM_data4]        (
 ; CHECK-NEXT:     [0x0000000000000000, 0x0000000000000007): DW_OP_reg5 RDI, DW_OP_piece 0x8, DW_OP_piece 0x4, DW_OP_reg4 RSI, DW_OP_piece 0x4
-; CHECK-NEXT:     [0x0000000000000007, 0x0000000000000007): DW_OP_reg5 RDI, DW_OP_piece 0x8, DW_OP_piece 0x4, DW_OP_reg0 RAX, DW_OP_piece 0x4)
 ; CHECK-NEXT:   DW_AT_name {{.*}}"outer"
 ; CHECK: DW_TAG_variable
-; CHECK-NEXT:   DW_AT_location [DW_FORM_data4]        (0x00000044
-; CHECK-NEXT:     [0x0000000000000007, 0x0000000000000007): DW_OP_reg0 RAX, DW_OP_piece 0x4)
-; CHECK-NEXT:   "i1"
+; CHECK-NEXT:   DW_AT_name {{.*}}"i1"
+; CHECK-NOT:    DW_AT_location
 
 ; ModuleID = '/Volumes/Data/llvm/test/DebugInfo/X86/sroasplit-2.ll'
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
 ; Function Attrs: nounwind ssp uwtable
 define i32 @foo(i64 %outer.coerce0, i64 %outer.coerce1) #0 !dbg !4 {
   call void @llvm.dbg.value(metadata i64 %outer.coerce0, metadata !24, metadata !25), !dbg !26