[DebugInfo] Do not generate address info for removed labels.

[HsiangKai]

Summary

In some senario, LLVM will remove llvm.dbg.labels in IR. For example, when the labels are in unreachable blocks, these labels will not be generated in LLVM IR. In the case, these debug labels will have address zero as their address. It is not legal address for debugger to set breakpoints or query sources. So, the patch inhibits the address info (DW_AT_low_pc) of removed labels.

[HsiangKai]

This patch is related to https://reviews.llvm.org/D50495#1225927.

[dblaikie]

Technically, I would think it might be best to produce the debug info for the label in the appropriate scope, but without an address - so that users can see that the label is there, name lookup finds that label not some other label or object?

[HsiangKai]

If it has no DW_AT_low_pc in label debug info, GDB seems to assume it is 0x0.

(gdb) info scope foo
Scope for foo:
Symbol done is a label at address 0x400551, length 8.
Symbol unreach is a label at address 0x0, length 8.

llvm-dwarfdump results:

0x00000056:     DW_TAG_label
                  DW_AT_abstract_origin (0x00000075 "done")
                  DW_AT_low_pc  (0x0000000000400551)

0x00000063:     DW_TAG_label
                  DW_AT_abstract_origin (0x0000007c "unreach")

0x00000068:     NULL
...
0x00000075:     DW_TAG_label
                  DW_AT_name    ("done")
                  DW_AT_decl_file       ("edd2.c")
                  DW_AT_decl_line       (13)

0x0000007c:     DW_TAG_label
                  DW_AT_name    ("unreach")
                  DW_AT_decl_file       ("edd2.c")
                  DW_AT_decl_line       (11)

0x00000083:     NULL

It will trigger assertions in GDB if we set breakpoint on labels with address 0x0 on x86 platform.

(gdb) b foo:done
Note: breakpoint 1 also set at pc 0x400581.
Breakpoint 2 at 0x400551: foo:done. (2 locations)
(gdb) list foo:done
8       int foo(void)
9       {
10          goto done;
11      unreach:
12          printf("hi\n");
13      done:
14          return rand() % 666;
15      }
16
17      int main()
(gdb) list foo:unreach
6       }
7
8       int foo(void)
9       {
10          goto done;
11      unreach:
12          printf("hi\n");
13      done:
14          return rand() % 666;
15      }
(gdb) b foo:unreach
/build/gdb-GT4MLW/gdb-8.1/gdb/linespec.c:3302: internal-error: void decode_line_full(const
 event_location*, int, program_space*, symtab*, int, linespec_result*, const char*, const
char*): Assertion `result.size () == 1 || canonical->pre_expanded' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

So, to avoid the assertion failed in GDB, I removed debug info of these unreachable labels in this patch.

[probinson]

Can you preserved the declaration DIE (at 0x0000007c) while omitting the definition DIE (at 0x00000063)? Does that still cause a problem for GDB?

[dblaikie]

In D51908#1231469, @HsiangKai wrote:

If it has no DW_AT_low_pc in label debug info, GDB seems to assume it is 0x0.

(gdb) info scope foo
Scope for foo:
Symbol done is a label at address 0x400551, length 8.
Symbol unreach is a label at address 0x0, length 8.

llvm-dwarfdump results:

0x00000056:     DW_TAG_label
                  DW_AT_abstract_origin (0x00000075 "done")

Hmm - what's the test case you're using here? Is there inlining? What about in cases without inlining?

[HsiangKai]

In D51908#1232585, @dblaikie wrote:

In D51908#1231469, @HsiangKai wrote:

If it has no DW_AT_low_pc in label debug info, GDB seems to assume it is 0x0.

(gdb) info scope foo
Scope for foo:
Symbol done is a label at address 0x400551, length 8.
Symbol unreach is a label at address 0x0, length 8.

llvm-dwarfdump results:

0x00000056:     DW_TAG_label
                  DW_AT_abstract_origin (0x00000075 "done")

Hmm - what's the test case you're using here? Is there inlining? What about in cases without inlining?

My test case is as follow. I compile it with optimization on, so there is inlining in the result.

int foo(void)
{
    goto done;
unreach:
    printf("hi\n");
done:
    return rand() % 666;
}

int main()
{
  return foo();
}

I removed the function call as follow:

int main()
{
  goto done;
unreach:
  printf("hi\n");
done:
  return rand() % 666;
}

llvm-dwarfdump results:

0x00000043:     DW_TAG_label
                  DW_AT_name    ("done")
                  DW_AT_decl_file       ("noinline.c")
                  DW_AT_decl_line       (9)
                  DW_AT_low_pc  (0x0000000000400541)

0x00000052:     DW_TAG_label
                  DW_AT_name    ("unreach")
                  DW_AT_decl_file       ("noinline.c")
                  DW_AT_decl_line       (7)

0x00000059:     NULL

It still caused GDB to crash if set breakpoint on unreach

[HsiangKai]

In D51908#1231568, @probinson wrote:

Can you preserved the declaration DIE (at 0x0000007c) while omitting the definition DIE (at 0x00000063)? Does that still cause a problem for GDB?

Yes, I tried it on no-inlined test case. It still caused a problem for GDB.

[vext01]

Hi,

It seems to me that the correct thing to do with unreachable labels is to emit a label with no low_pc attribute and fix gdb to accept such labels. Instead of crashing, it should print a message like "label was optimised out".

(It also shouldn't crash if a label has a zero low_pc value. I guess that zero could (in esoteric scenarios) be a valid code address. Perhaps for raw (headerless) binaries on some system which doesn't relocate code at load time?).

If you guys agree, I don't mind raising the bug, but we'd need an easily reproducible crash case to show them.

[dblaikie]

In D51908#1232920, @vext01 wrote:

It seems to me that the correct thing to do with unreachable labels is to emit a label with no low_pc attribute and fix gdb to accept such labels. Instead of crashing, it should print a message like "label was optimised out".

Agreed, though producing debug info that's likely to break any GDB in the interim's not ideal either - I'd certainly encourage folks to file the GDB bug and encourage a discussion with GDB folks about whether a label with no low_pc attribute is the right representation (I think it is, but open to other ideas)

(It also shouldn't crash if a label has a zero low_pc value. I guess that zero could (in esoteric scenarios) be a valid code address. Perhaps for raw (headerless) binaries on some system which doesn't relocate code at load time?).

Agreed

If you guys agree, I don't mind raising the bug, but we'd need an easily reproducible crash case to show them.

*nod* looks like that's been provided elsewhere in this review (though I've not gone through the repro steps myself).

Perhaps we could implement this behavior only if we're tuning for GDB & otherwise produce a label without a low_pc? Not sure how other folks (@aprantl @echristo @probinson @JDevlieghere) feel about this - apply the workaround generally, because it's not super important to have a label for name-lookup porpoises? or is it worth carrying the two codepaths to ensure that slightly more accurate debug info where it's possible (presuming LLDB can cope with it)?

[probinson]

In D51908#1233588, @dblaikie wrote:

Perhaps we could implement this behavior (pr: i.e., omitting the label if it has been optimized away) only if we're tuning for GDB & otherwise produce a label without a low_pc? Not sure how other folks (@aprantl @echristo @probinson @JDevlieghere) feel about this - apply the workaround generally, because it's not super important to have a label for name-lookup porpoises? or is it worth carrying the two codepaths to ensure that slightly more accurate debug info where it's possible (presuming LLDB can cope with it)?

Special cases for GDB bugs are not great. While on principle I'd rather see the entry with no location, I agree it's not super important to keep it and it's less grody to behave the same for all tunings here.

[HsiangKai]

I use the same test program to generate executable with gcc. The executable has DW_TAG_label without DW_AT_low_pc. However, it will not crash GDB. There should be other debug attributes that will affect the behavior as setting breakpoints on labels.

I will dig into the difference and report here if I found the root cause.

[HsiangKai]

In D51908#1237844, @HsiangKai wrote:

I use the same test program to generate executable with gcc. The executable has DW_TAG_label without DW_AT_low_pc. However, it will not crash GDB. There should be other debug attributes that will affect the behavior as setting breakpoints on labels.

I will dig into the difference and report here if I found the root cause.

By default, gcc will generate relocatable executable. So, when users set breakpoints on labels without address, address zero will be relocated to load address. It is not correct locations for labels. However, it will not cause gdb to crash. If I compile the test case with -no-pie, it will encounter the same problem.

[HsiangKai]

I think that all of us agreed that it is a bug in GDB to crash when setting breakpoints on labels without address information. So, I updated the patch to generate label debug information without address even when the labels are removed after optimizations.

[probinson]

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

871

Fold the if and getSymbol together:

if (Label)
  if (const MCSymbol *Sym = Label->getSymbol())
    addLabelAddress(*Die, dwarf::DW_AT_low_pc, Sym);

You could maybe use auto * as well, instead of spelling out the type.

test/DebugInfo/Generic/debug-label-unreached.ll

8

Make sure the low_pc is part of the same tag by putting this in front of it:
; CHECK-NOT: {{DW_TAG|NULL}}

[probinson]

LGTM

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (AbsEntity && AbsEntity->getDIE()) {
     else if ((Label = dyn_cast<const DbgLabel>(Entity)))
       applyLabelAttributes(*Label, *Die);
     else
       llvm_unreachable("DbgEntity must be DbgVariable or DbgLabel.");
   }
 
   if (Label) {
     const MCSymbol *Sym = Label->getSymbol();
+    if (Sym)
       addLabelAddress(*Die, dwarf::DW_AT_low_pc, Sym);
   }
 }
 
 DbgEntity *DwarfCompileUnit::getExistingAbstractEntity(const DINode *Node) {
   auto &AbstractEntities = getAbstractEntities();
   auto I = AbstractEntities.find(Node);
   if (I != AbstractEntities.end())
     return I->second.get();

test/DebugInfo/Generic/debug-label-unreached.ll

+; Test unreachable llvm.dbg.label
+;
+; RUN: llc -filetype=obj -split-dwarf-file debug.dwo -o - %s | llvm-dwarfdump -v - | FileCheck %s
+;
+; CHECK: .debug_info.dwo contents:
+; CHECK: DW_TAG_label
+; CHECK-NEXT: DW_AT_name {{.*}}"done"
+; CHECK: DW_AT_low_pc
+; CHECK: DW_TAG_label
+; CHECK-NEXT: DW_AT_name {{.*}}"removed"
+; CHECK-NOT: DW_AT_low_pc
+source_filename = "debug-label-unreached.c"
+
+define dso_local i32 @foo(i32 %a, i32 %b) !dbg !8 {
+entry:
+  %sum = add nsw i32 %a, %b, !dbg !12
+  call void @llvm.dbg.label(metadata !11), !dbg !12
+  ret i32 %sum, !dbg !13
+}
+
+declare void @llvm.dbg.label(metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: true, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "debug-label-unreached.c", directory: "./")
+!2 = !{}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !DISubroutineType(types: !6)
+!6 = !{!7}
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 8, type: !5, isLocal: false, isDefinition: true, scopeLine: 9, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !9)
+!9 = !{!10, !11}
+!10 = !DILabel(scope: !8, name: "removed", file: !1, line: 11)
+!11 = !DILabel(scope: !8, name: "done", file: !1, line: 13)
+!12 = !DILocation(line: 13, column: 1, scope: !8)
+!13 = !DILocation(line: 14, column: 5, scope: !8)