Fix DwarfDebug assertions with LTO mixing -g and -gmlt

[probinson]

Summary

When LTO inlines functions compiled with -g into functions compiled
-gmlt, certain assumptions don't hold any more.

Fixes PR31437.

[dblaikie]

Given the complexity of the test cases, this might be easier as two separate reviews.

The test cases should be simplified a fair bit further, I think - especially removing user defined types should be helpful in reducing the amount of debug info metadata and possibly making things clearer. (also, in the second test it mentions the possibility of removing the (non-inlined) definition of 'foo' - mark 'foo' as inline in the source and this should happen automatically. If possible, use attribute((always_inline)) to ensure the test case is fairly optimization independent, but I guess since it needs to optimize away the parameter alloca it won't be doable at -O0 anyway, so maybe that's not important)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1151 ↗

(On Diff #86356)

Rather than adding a new member variable & diagnosing this case where we were before - perhaps it'd make for a more actionable assertion if we change how we're doing this.

There's exactly one place that's (admittedly, currently - this could change, so might make what I'm proposing more brittle) creating AbstractVariables - in DwarfDebug::createAbstractVariable.

From that function it should be possible to check the debug info level (CurFn->getFunction()->getSubprogram()->getUnit()->getEmissionKind()) and assert that it is not LineTablesOnly. That way it'll fail faster/more informatively, and requires less wide-scoped state tracking, etc.

1225–1229 ↗

(On Diff #86356)

Do bad things happen if we don't clear this? Might be useful if the comment described what undesirable work this avoids?

[probinson]

I was wondering whether it might be too much for one review. Easy enough to break it up.

I've already spent a bunch of time trying to reduce the test cases, but I'll have another go at it.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1151 ↗

(On Diff #86356)

I'd be ecstatic to avoid the extra state. I was really not happy about that.
However it's not actually wrong to create abstract variables in a -gmlt function, *if* that function has inlined a -g function. I was thinking maybe createAbstractVariable should assert that there are abstract scopes, but looking at the paths into that method, you can only get there if you already have an abstract scope (so the assertion would be pretty pointless).
Should we just abandon the original assertion?

1225–1229 ↗

(On Diff #86356)

Hmmm maybe nothing bad happens, and we just eliminate the assertion.
DbgValues won't generate any line-table entries, and we've already seen that they won't generate any abstract scopes, so maybe it's benign to just ignore them?

[dblaikie]

test/DebugInfo/X86/dbg-abstract-vars-g-gmlt.ll

1 ↗

(On Diff #86356)

Here's the simplest example I have for this (also doesn't require optimizations either at compile time or after the link step (though they can be used to tidy up the IR a little):

// first file
void f1();
inline __attribute__((always_inline)) void f2(int) {
  f1();
}
void f3() {
  f2();
}

// second file
void f() {
}

test/DebugInfo/X86/dbg-value-g-gmlt.ll

1 ↗

(On Diff #86356)

Interestingly, this still seems to trigger the assert:

void f()
void foo(int param) {
  if (param)
    f();
}

void foo(int);
void bar() {
  foo(0);
}

Which is vaguely worrying - since in this case the call to foo is not/cannot be optimized away, so I'm not sure what the assertion is telling us here. There should be instructions for the function & so we shouldn't be in that codepath.

[probinson]

test/DebugInfo/X86/dbg-abstract-vars-g-gmlt.ll

1 ↗

(On Diff #86356)

I have not been able to get your example to crash on an unmodified compiler, can you provide more details?

test/DebugInfo/X86/dbg-value-g-gmlt.ll

1 ↗

(On Diff #86356)

Why can't it be inlined away? bar() calls foo(), foo() gets inlined into bar(), the constant 0 parameter is propagated to the 'if', the condition is guaranteed false therefore the 'if/then' goes away, poof all gone. Except for the dbg.value describing the inlined copy of 'param'.

[probinson]

Reduce patch to just the DbgValues.empty() assertion; the other will become a new review.
Use dblaikie's simpler test case.

I've just deleted the assertion, because we call DebugHandlerBase::endFunction() shortly after this, which will clear DbgValues for us.

[dblaikie]

Looks good - thanks!

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

void DwarfDebug::endFunction(const MachineFunction *MF) {
 
   // Under -gmlt, skip building the subprogram if there are no inlined
   // subroutines inside it. But with -fdebug-info-for-profiling, the subprogram
   // is still needed as we need its source location.
   if (!Asm->TM.Options.DebugInfoForProfiling &&
       TheCU.getCUNode()->getEmissionKind() == DICompileUnit::LineTablesOnly &&
       LScopes.getAbstractScopesList().empty() && !IsDarwin) {
     assert(InfoHolder.getScopeVariables().empty());
-    assert(DbgValues.empty());
     // FIXME: This wouldn't be true in LTO with a -g (with inlining) CU followed
     // by a -gmlt CU. Add a test and remove this assertion.
     assert(AbstractVariables.empty());
     PrevLabel = nullptr;
     CurFn = nullptr;
     DebugHandlerBase::endFunction(MF);
     return;
   }

llvm/trunk/test/DebugInfo/X86/dbg-value-g-gmlt.ll

+; RUN: llc < %s -filetype=obj | llvm-dwarfdump - -debug-dump=info | FileCheck %s
+;
+; IR module created as follows:
+;   clang -emit-llvm -S -O2 foo.cpp -o foo.ll -g
+;   clang -emit-llvm -S -O2 bar.cpp -o bar.ll -gmlt
+;   llvm-link foo.ll bar.ll -S -o linked.ll
+;   opt -std-link-opts linked.ll -S -o opt.ll
+; --- foo.cpp ---
+; void f();
+; void foo(int param) {
+;   if (param) f();
+; }
+; --- bar.cpp ---
+; void foo(int);
+; void bar() {
+;   foo(0);
+; }
+; ---
+; The point is that bar() is compiled -gmlt and calls foo() with a constant 0.
+; foo() is compiled -g and gets inlined into bar(); foo's body is then
+; optimized away, leaving only a dbg.value call describing the inlined copy
+; of 'param', which should be benign.
+; That is, the compile-unit for bar.cpp should have nothing in it.
+; PR31437.
+
+; foo.cpp's unit comes first; skip past it, second unit should be empty.
+; CHECK:     DW_TAG_compile_unit
+; CHECK-NOT: {{DW_TAG|NULL}}
+; CHECK:     DW_AT_name {{.*}} "foo.cpp"
+; CHECK:     DW_TAG_compile_unit
+; CHECK-NOT: DW_TAG
+
+; ModuleID = 'linked.ll'
+source_filename = "llvm-link"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: uwtable
+define void @_Z3fooi(i32 %param) local_unnamed_addr #0 !dbg !8 {
+entry:
+  tail call void @llvm.dbg.value(metadata i32 %param, i64 0, metadata !13, metadata !14), !dbg !15
+  %tobool = icmp eq i32 %param, 0, !dbg !16
+  br i1 %tobool, label %if.end, label %if.then, !dbg !18
+
+if.then:                                          ; preds = %entry
+  tail call void @_Z1fv(), !dbg !19
+  br label %if.end, !dbg !19
+
+if.end:                                           ; preds = %if.then, %entry
+  ret void, !dbg !21
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
+
+declare void @_Z1fv() local_unnamed_addr #2
+
+; Function Attrs: nounwind readnone uwtable
+define void @_Z3barv() local_unnamed_addr #3 !dbg !22 {
+entry:
+  tail call void @llvm.dbg.value(metadata i32 0, i64 0, metadata !13, metadata !14), !dbg !24
+  ret void, !dbg !26
+}
+
+attributes #0 = { uwtable }
+attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind }
+attributes #3 = { nounwind readnone uwtable }
+
+!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 293745)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "foo.cpp", directory: "/home/probinson/projects/scratch/pr31437")
+!2 = !{}
+!3 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !4, producer: "clang version 5.0.0 (trunk 293745)", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
+!4 = !DIFile(filename: "bar.cpp", directory: "/home/probinson/projects/scratch/pr31437")
+!5 = !{!"clang version 5.0.0 (trunk 293745)"}
+!6 = !{i32 2, !"Dwarf Version", i32 4}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = distinct !DISubprogram(name: "foo", linkageName: "_Z3fooi", scope: !1, file: !1, line: 2, type: !9, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13}
+!13 = !DILocalVariable(name: "param", arg: 1, scope: !8, file: !1, line: 2, type: !11)
+!14 = !DIExpression()
+!15 = !DILocation(line: 2, column: 14, scope: !8)
+!16 = !DILocation(line: 3, column: 7, scope: !17)
+!17 = distinct !DILexicalBlock(scope: !8, file: !1, line: 3, column: 7)
+!18 = !DILocation(line: 3, column: 7, scope: !8)
+!19 = !DILocation(line: 3, column: 14, scope: !20)
+!20 = !DILexicalBlockFile(scope: !17, file: !1, discriminator: 1)
+!21 = !DILocation(line: 4, column: 1, scope: !8)
+!22 = distinct !DISubprogram(name: "bar", scope: !4, file: !4, line: 2, type: !23, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !3, variables: !2)
+!23 = !DISubroutineType(types: !2)
+!24 = !DILocation(line: 2, column: 14, scope: !8, inlinedAt: !25)
+!25 = distinct !DILocation(line: 3, column: 3, scope: !22)
+!26 = !DILocation(line: 4, column: 1, scope: !22)