Handle link of NoDebug CU with a CU that has debug emission enabled

[tejohnson]

Summary

This is an issue both with regular and Thin LTO. When we link together
a DICompileUnit that is marked NoDebug (e.g when compiling with -g0
but applying an AutoFDO profile, which requires location tracking
in the compiler) and a DICompileUnit with debug emission enabled,
we can have failures during dwarf debug generation. Specifically,
when we have inlined from the NoDebug compile unit into the debug
compile unit, we can fail during construction of the abstract and
inlined scope DIEs. This is because the SPMap does not include NoDebug
CUs (they are skipped in the debug_compile_units_iterator).

This patch fixes the failures by skipping SPs from NoDebug CUs when
generating these DIEs. I am not sure if this is the right fix from
the debug generation standpoint, however. I don't know if we should
instead either upgrade NoDebug CU on link with a debug CU, or enable
adding these NoDebug CUs into the SPMap.

[dblaikie]

Superficially (I haven't looked at all the codepaths, etc) the code change looks OK.

Testing could probably be a lot simpler - can you take the merged IR and test that, rather than running LTO steps in the test? It seems the resulting IR is what we want to test/what's being fixed, not the path that was used to reach it.

[tejohnson]

In D29765#672165, @dblaikie wrote:

Superficially (I haven't looked at all the codepaths, etc) the code change looks OK.

Testing could probably be a lot simpler - can you take the merged IR and test that, rather than running LTO steps in the test? It seems the resulting IR is what we want to test/what's being fixed, not the path that was used to reach it.

Great idea. Uploading patch with new test now.

[tejohnson]

Change to a test that is already combined.

[dblaikie]

Are the locations properly respected/ignored in this case? (what does the line table end up looking like for the instructions in the nodebug inlined code?)

Would it be worth/possible omitting these scopes from the LexicalScope tree in the first place, rather than filtering out here after-the-fact?

test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

3 ↗

(On Diff #87834)

Include comments about the source code and commands to reproduce the IR (since debug info metadata is a bit hard to read, this can help with understanding exactly what the test is testing, reproducing, etc)

[tejohnson]

In D29765#672324, @dblaikie wrote:

Are the locations properly respected/ignored in this case? (what does the line table end up looking like for the instructions in the nodebug inlined code?)

Here is the result of objdump -WL (let me know if there is something else you wanted to see):

$ objdump -WL a.out

a.out: file format elf64-x86-64

Decoded dump of debug contents of section .debug_line:

CU: debug.c:
File name Line number Starting address
debug.c 2 0

./nodebug.c:[++]
nodebug.c 3 0x4

./debug.c:[++]
debug.c 2 0x10

Would it be worth/possible omitting these scopes from the LexicalScope tree in the first place, rather than filtering out here after-the-fact?

That I am not sure about, as I'm not really familiar with this code. I am hoping you will tell me. =)

test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

3 ↗

(On Diff #87834)

Will do.

[dblaikie]

In D29765#672333, @tejohnson wrote:

In D29765#672324, @dblaikie wrote:

Are the locations properly respected/ignored in this case? (what does the line table end up looking like for the instructions in the nodebug inlined code?)

Here is the result of objdump -WL (let me know if there is something else you wanted to see):

$ objdump -WL a.out

a.out: file format elf64-x86-64

Decoded dump of debug contents of section .debug_line:

CU: debug.c:
File name Line number Starting address
debug.c 2 0

./nodebug.c:[++]
nodebug.c 3 0x4

./debug.c:[++]
debug.c 2 0x10

Ah, I meant more the content of the line table (llvm-dwarfdump -debug-dump=line should show you this).

Would it be worth/possible omitting these scopes from the LexicalScope tree in the first place, rather than filtering out here after-the-fact?

That I am not sure about, as I'm not really familiar with this code. I am hoping you will tell me. =)

I think the right place is /probably/ LexicalScopes::extractLexicalScopes (lib/CodeGen/LexicalScopes.cpp) - if it'

[dblaikie]

Oops, sent incomplete message.

But, yes, in LexicalScopes::extractLexicalScopes ideally, I think, it would not consider locations that aren't intended for debug info.

[tejohnson]

In D29765#673762, @dblaikie wrote:

In D29765#672333, @tejohnson wrote:

In D29765#672324, @dblaikie wrote:

Are the locations properly respected/ignored in this case? (what does the line table end up looking like for the instructions in the nodebug inlined code?)

Here is the result of objdump -WL (let me know if there is something else you wanted to see):

$ objdump -WL a.out

a.out: file format elf64-x86-64

Decoded dump of debug contents of section .debug_line:

CU: debug.c:
File name Line number Starting address
debug.c 2 0

./nodebug.c:[++]
nodebug.c 3 0x4

./debug.c:[++]
debug.c 2 0x10

Ah, I meant more the content of the line table (llvm-dwarfdump -debug-dump=line should show you this).

Here is the output of running the above on the new a.out generated from the included test case (with the new version of the patch I will upload momentarily):

a.out: file format ELF64-x86-64

.debug_line contents:
Line table prologue:

total_length: 0x0000004d
     version: 2

prologue_length: 0x0000002b
min_inst_length: 1
default_is_stmt: 1

  line_base: -5
 line_range: 14
opcode_base: 13

standard_opcode_lengths[DW_LNS_copy] = 0
standard_opcode_lengths[DW_LNS_advance_pc] = 1
standard_opcode_lengths[DW_LNS_advance_line] = 1
standard_opcode_lengths[DW_LNS_set_file] = 1
standard_opcode_lengths[DW_LNS_set_column] = 1
standard_opcode_lengths[DW_LNS_negate_stmt] = 0
standard_opcode_lengths[DW_LNS_set_basic_block] = 0
standard_opcode_lengths[DW_LNS_const_add_pc] = 0
standard_opcode_lengths[DW_LNS_fixed_advance_pc] = 1
standard_opcode_lengths[DW_LNS_set_prologue_end] = 0
standard_opcode_lengths[DW_LNS_set_epilogue_begin] = 0
standard_opcode_lengths[DW_LNS_set_isa] = 1

Dir  Mod Time   File Len   File Name
---- ---------- ---------- ---------------------------

file_names[ 1] 0 0x00000000 0x00000000 debug.c
file_names[ 2] 0 0x00000000 0x00000000 nodebug.c

Address Line Column File ISA Discriminator Flags

---

0x0000000000000000 2 0 1 0 0 is_stmt
0x0000000000000004 3 1 2 0 0 is_stmt prologue_end
0x0000000000000010 2 21 1 0 0 is_stmt
0x0000000000000014 2 21 1 0 0 is_stmt end_sequence

Would it be worth/possible omitting these scopes from the LexicalScope tree in the first place, rather than filtering out here after-the-fact?

That I am not sure about, as I'm not really familiar with this code. I am hoping you will tell me. =)

I think the right place is /probably/ LexicalScopes::extractLexicalScopes (lib/CodeGen/LexicalScopes.cpp) - if it'

Uploading new patch that does that instead.

[tejohnson]

Address comments

[dblaikie]

lib/CodeGen/LexicalScopes.cpp

82 ↗

(On Diff #88285)

This probably won't catch all cases - if the location was in a subscope within the function (eg: try adding a {} to the function which should add an extra scope). Walking the scope chain until a DISubprogram is probably needed here - I forget if we have a nice wrapped up utility for that. Don't see one at a glance.

It should be an invariant/guaranteed that a DISubprogram is reached (& is the end of the scope chain), so something like:

oh, wait, no, here it is: http://llvm.org/docs/doxygen/html/classllvm_1_1DILocalScope.html#a747dd1690fc477a8d9638fa37a30ced8

so, I think, maybe:

if (cast<DILocalScope>(MIDL->getScope())->getSubprogram()->getUnit()->getEmissionKind() == DICompileUnit::NoDebug) {
  ...

test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

4 ↗

(On Diff #88285)

Include the steps for reproducing this test case (basically the RUN lines, and the original C source/steps for producing the IR, from the first version of the test) - helps to visualize/document/reproduce the situation.

[tejohnson]

lib/CodeGen/LexicalScopes.cpp

82 ↗

(On Diff #88285)

I had just found the DILocalScope::getSubprogram() helper, since I was still getting the original seg fault when I tried the original failing internal code with this change. So now I have:

// Ignore locations linked from a NoDebug compile unit.
auto *SP = MIDL->getScope()->getSubprogram();
if (SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug) {
  PrevMI = &MInsn;
  continue;
}

(Don't need to cast MIDL->getScope() since it returns a DILocalScope). But I am *still* getting the original seg fault. Any idea what else I could be missing?

test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

4 ↗

(On Diff #88285)

Will do

[echristo]

One drive by comment while I'm looking at it.

test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

25 ↗

(On Diff #88285)

Clean up attributes :)

[tejohnson]

Address comments and apply discussed fixes. Update test.

[dblaikie]

I was hoping to simplify this a bit by having DwarfDebug (well, DebugHandlerBase) filter and never try to build a LexicalScopes for a non-debug-having function (so it wouldn't need the filter at the start of LexicalScopes::initialize), except LiveDebugValues also creates LexicalScopes and doesn't have any existing filtering/probably shouldn't have any.

So I'm back around to basically this change, with a couple of questions.

lib/CodeGen/LexicalScopes.cpp

87–94 ↗

(On Diff #88597)

Which test case/behavior does this address that the change in getOrCreateLexicalScope wasn't able to handle?

182–184 ↗

(On Diff #88597)

What about asserting in LexicalScope's ctor?

This was what I tried & seemed to provide the checking desired?

diff --git include/llvm/CodeGen/LexicalScopes.h include/llvm/CodeGen/LexicalScopes.h
index 7d7e48af2a0..b6d524fce44 100644

• include/llvm/CodeGen/LexicalScopes.h

+++ include/llvm/CodeGen/LexicalScopes.h
@@ -49,7 +49,11 @@ public:

         bool A)
: Parent(P), Desc(D), InlinedAtLocation(I), AbstractScope(A),
  LastInsn(nullptr), FirstInsn(nullptr), DFSIn(0), DFSOut(0) {

• assert((!D || D->isResolved()) && "Expected resolved node");

+ assert(D);
+ assert(D->getSubprogram()->getUnit()->getEmissionKind() !=
+ DICompileUnit::NoDebug &&
+ "Don't build lexical scopes for non-debug locations");
+ assert(D->isResolved() && "Expected resolved node");

assert((!I || I->isResolved()) && "Expected resolved node");
if (Parent)
  Parent->addChild(this);

[tejohnson]

lib/CodeGen/LexicalScopes.cpp

87–94 ↗

(On Diff #88597)

Good point, I think it doesn't add anything now (and now that I am looking at this I realize I forgot to change it to use getSubprogram() rather than use the dyn_cast). Let me try removing that.

182–184 ↗

(On Diff #88597)

Good idea, will do that instead.

[tejohnson]

Address review comments

[dblaikie]

Add the assert to LexicalScope's ctor and (assuming that works) this should be good.

[tejohnson]

In D29765#679062, @dblaikie wrote:

Add the assert to LexicalScope's ctor and (assuming that works) this should be good.

It works - it is there in the new patch I uploaded.

[dblaikie]

In D29765#679066, @tejohnson wrote:

In D29765#679062, @dblaikie wrote:

Add the assert to LexicalScope's ctor and (assuming that works) this should be good.

It works - it is there in the new patch I uploaded.

Ah, yeah - blindness on my side. (commit whenever you're ready)

[echristo]

LGTM as well. Thanks for the thorough work here both of you. :)

llvm/trunk/include/llvm/CodeGen/LexicalScopes.h

 ///
 class LexicalScope {
 
 public:
   LexicalScope(LexicalScope *P, const DILocalScope *D, const DILocation *I,
                bool A)
       : Parent(P), Desc(D), InlinedAtLocation(I), AbstractScope(A),
         LastInsn(nullptr), FirstInsn(nullptr), DFSIn(0), DFSOut(0) {
-    assert((!D || D->isResolved()) && "Expected resolved node");
+    assert(D);
+    assert(D->getSubprogram()->getUnit()->getEmissionKind() !=
+           DICompileUnit::NoDebug &&
+           "Don't build lexical scopes for non-debug locations");
+    assert(D->isResolved() && "Expected resolved node");
     assert((!I || I->isResolved()) && "Expected resolved node");
     if (Parent)
       Parent->addChild(this);
   }
 
   // Accessors.
   LexicalScope *getParent() const { return Parent; }
   const MDNode *getDesc() const { return Desc; }

llvm/trunk/lib/CodeGen/LexicalScopes.cpp

void LexicalScopes::reset() {
   LexicalScopeMap.clear();
   AbstractScopeMap.clear();
   InlinedLexicalScopeMap.clear();
   AbstractScopesList.clear();
 }
 
 /// initialize - Scan machine function and constuct lexical scope nest.
 void LexicalScopes::initialize(const MachineFunction &Fn) {
+  // Don't attempt any lexical scope creation for a NoDebug compile unit.
+  if (Fn.getFunction()->getSubprogram()->getUnit()->getEmissionKind() ==
+      DICompileUnit::NoDebug)
+    return;
   reset();
   MF = &Fn;
   SmallVector<InsnRange, 4> MIRanges;
   DenseMap<const MachineInstr *, LexicalScope *> MI2ScopeMap;
   extractLexicalScopes(MIRanges, MI2ScopeMap);
   if (CurrentFnLexicalScope) {
     constructScopeNest(CurrentFnLexicalScope);
     assignInstructionRanges(MIRanges, MI2ScopeMap);
LexicalScope *LexicalScopes::findLexicalScope(const DILocation *DL) {
   return findLexicalScope(Scope);
 }
 
 /// getOrCreateLexicalScope - Find lexical scope for the given DebugLoc. If
 /// not available then create new lexical scope.
 LexicalScope *LexicalScopes::getOrCreateLexicalScope(const DILocalScope *Scope,
                                                      const DILocation *IA) {
   if (IA) {
+    // Skip scopes inlined from a NoDebug compile unit.
+    if (Scope->getSubprogram()->getUnit()->getEmissionKind() ==
+        DICompileUnit::NoDebug)
+      return getOrCreateLexicalScope(IA);
     // Create an abstract scope for inlined function.
     getOrCreateAbstractScope(Scope);
     // Create an inlined scope for inlined function.
     return getOrCreateInlinedScope(Scope, IA);
   }
 
   return getOrCreateRegularScope(Scope);
 }

llvm/trunk/test/DebugInfo/Generic/debug_and_nodebug_CUs.ll

+; Test to ensure that a module containing both a NoDebug CU and one with
+; debug is handled correctly.
+
+; LLVM IR was generated the following way:
+; $ cat a.cpp
+; void f1();
+; __attribute__((always_inline)) void f2() {
+;     f1();
+; }
+; void f3();
+; void f4() {
+;     f3();
+; }
+; $ cat b.cpp
+; void f2();
+; __attribute__((always_inline)) void f3() {
+;     f2();
+; }
+; $ clang++ -flto a.cpp -g -c
+; $ clang++ -flto b.cpp -Rpass=inline -c
+; $ llvm-link {a,b}.o -o - | opt -O2 - -o ab.bc
+; $ llvm-dis ab.bc
+
+; Ensure we can successfully generate assembly, and check that neither
+; "b.cpp" nor "f3" strings show up (which would be in the .debug_str
+; section if we had generated any lexical scopes and debug for them).
+; RUN: llc %s -o - | FileCheck %s
+; CHECK-NOT: .asciz  "b.cpp"
+; CHECK-NOT: .asciz  "f3"
+
+; ModuleID = 'debug_and_nodebug_CUs.bc'
+source_filename = "debug_and_nodebug_CUs.ll"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @_Z2f2v() local_unnamed_addr !dbg !8 {
+entry:
+  tail call void @_Z2f1v(), !dbg !11
+  ret void, !dbg !12
+}
+
+declare void @_Z2f1v() local_unnamed_addr
+
+define void @_Z2f4v() local_unnamed_addr !dbg !13 {
+entry:
+  tail call void @_Z2f1v(), !dbg !14
+  ret void, !dbg !19
+}
+
+define void @_Z2f3v() local_unnamed_addr !dbg !16 {
+entry:
+  tail call void @_Z2f1v(), !dbg !20
+  ret void, !dbg !22
+}
+
+!llvm.dbg.cu = !{!0, !3}
+!llvm.ident = !{!5, !5}
+!llvm.module.flags = !{!6, !7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 5.0.0 (trunk 294362) (llvm/trunk 294367)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "a.cpp", directory: ".")
+!2 = !{}
+!3 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !4, producer: "clang version 5.0.0 (trunk 294362) (llvm/trunk 294367)", isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, enums: !2)
+!4 = !DIFile(filename: "b.cpp", directory: ".")
+!5 = !{!"clang version 5.0.0 (trunk 294362) (llvm/trunk 294367)"}
+!6 = !{i32 2, !"Dwarf Version", i32 4}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = distinct !DISubprogram(name: "f2", linkageName: "_Z2f2v", scope: !1, file: !1, line: 2, type: !9, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !DILocation(line: 3, column: 3, scope: !8)
+!12 = !DILocation(line: 4, column: 1, scope: !8)
+!13 = distinct !DISubprogram(name: "f4", linkageName: "_Z2f4v", scope: !1, file: !1, line: 6, type: !9, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!14 = !DILocation(line: 3, column: 3, scope: !8, inlinedAt: !15)
+!15 = distinct !DILocation(line: 3, column: 3, scope: !16, inlinedAt: !18)
+!16 = distinct !DISubprogram(name: "f3", scope: !4, file: !4, line: 2, type: !17, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !3, variables: !2)
+!17 = !DISubroutineType(types: !2)
+!18 = distinct !DILocation(line: 7, column: 3, scope: !13)
+!19 = !DILocation(line: 8, column: 1, scope: !13)
+!20 = !DILocation(line: 3, column: 3, scope: !8, inlinedAt: !21)
+!21 = distinct !DILocation(line: 3, column: 3, scope: !16)
+!22 = !DILocation(line: 4, column: 1, scope: !16)