[ThinLTO] Fix a crash in lazy loading of Metadata

[tejohnson]

Summary

This is a revised version of D41474.

When the debug location is parsed in BitcodeReader::parseFunction, the
scope and inlinedAt MDNodes are obtained via MDLoader->getMDNodeFwdRefOrNull(),
which will create a forward ref if they were not yet loaded.
Specifically, if one of these MDNodes is in the module level metadata
block, and this is during ThinLTO importing, that metadata block is
lazily loaded.

Most places in that invoke getMDNodeFwdRefOrNull have a corresponding call
to resolveForwardRefsAndPlaceholders which will take care of resolving them.
E.g. places that call getMetadataFwdRefOrLoad, or at the end of parsing a
function-level metadata block, or at the end of the initial lazy load of
module level metadata in order to handle invocations of getMDNodeFwdRefOrNull
for named metadata and global object attachments. However, the calls for
the scope/inlinedAt of debug locations are not backed by any such call to
resolveForwardRefsAndPlaceholders.

To fix this, change the scope and inlinedAt parsing to instead use
getMetadataFwdRefOrLoad, which will ensure the forward refs to lazily
loaded metadata are resolved.

Fixes PR35472.

[tejohnson]

Added part of test case missed in first version of patch.

[vsk]

Thanks for the patch! Your explanation makes sense to me, but I'm not familiar enough with this code to give a +1. @steven_wu, any thoughts on this?

[tejohnson]

Yikes! Accidental commit! Reverting...

[tejohnson]

Reverted in r345097.

[steven_wu]

In D53596#1272986, @vsk wrote:

Thanks for the patch! Your explanation makes sense to me, but I'm not familiar enough with this code to give a +1. @steven_wu, any thoughts on this?

Looks fine but I don't know enough about debug info to comment on this.

[tejohnson]

ping - @dexonsmith , can you take a look?

[vsk]

Hi Teresa, with this patch applied, I do still see a (possibly unrelated) crash when building an internal framework with ThinLTO + hot/cold splitting. I'm not sure what the best way is to prepare a reproducer (please let me know), but I've tried to collect some output from the debugger:

Foundation-fmwk-ThinLTO-crash.txt29 KBDownload

[tejohnson]

In D53596#1282167, @vsk wrote:

Hi Teresa, with this patch applied, I do still see a (possibly unrelated) crash when building an internal framework with ThinLTO + hot/cold splitting. I'm not sure what the best way is to prepare a reproducer (please let me know), but I've tried to collect some output from the debugger:

Foundation-fmwk-ThinLTO-crash.txt29 KBDownload

Doesn't look like this one has anything to do with the issue I'm fixing here. The stack trace shows it is coming from the call to materializeMetadata() from FunctionImport.cpp, and that is called before we invoke the IRMover (which is where we encountered the code with the bug, and the function changed by my patch). The code changed in this patch is not reached via materializeMetadata.

Can you file a new bug for the problem you encountered, along with some kind of reproducer? Looks like it might be hitting infinite, or at least very deep, recursion and running out of stack.

But what version are you on? The stack trace says the call to materializeMetadata is on line 765 of FunctionImport.cpp, and it is currently nowhere near that location and hasn't been in awhile.

[vsk]

In D53596#1282480, @tejohnson wrote:

In D53596#1282167, @vsk wrote:

Hi Teresa, with this patch applied, I do still see a (possibly unrelated) crash when building an internal framework with ThinLTO + hot/cold splitting. I'm not sure what the best way is to prepare a reproducer (please let me know), but I've tried to collect some output from the debugger:

Foundation-fmwk-ThinLTO-crash.txt29 KBDownload

Doesn't look like this one has anything to do with the issue I'm fixing here. The stack trace shows it is coming from the call to materializeMetadata() from FunctionImport.cpp, and that is called before we invoke the IRMover (which is where we encountered the code with the bug, and the function changed by my patch). The code changed in this patch is not reached via materializeMetadata.

Can you file a new bug for the problem you encountered, along with some kind of reproducer? Looks like it might be hitting infinite, or at least very deep, recursion and running out of stack.

But what version are you on? The stack trace says the call to materializeMetadata is on line 765 of FunctionImport.cpp, and it is currently nowhere near that location and hasn't been in awhile.

Thanks for looking. I'm using AppleClang-1000 (derived from llvm-6.0), as it's the version currently used to build our OSes. I'll work on finding a reproducer against the top-of-tree compiler.

[tejohnson]

Ping - can someone review/approve this? Suggestions on who else to review?

[steven_wu]

Oh, I see. This is not really related to the debug information quality. It is a pure metadata lazy loading problem. LGTM.

The only suggestion I have (maybe can be done in a separate commit) is that none of the call site of getMDNodeFwdRefOrNull are expecting null as a valid output because they all check for nullptr and error out in some way. Maybe this function should really be:

Error<MDNode*> getMDNodeFwdRef(unsigned);

And it can return a better error message.

[dexonsmith]

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

[dexonsmith]

In D53596#1287633, @dexonsmith wrote:

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

Sorry, I neglected to say anything actionable.

• If any bitcode writer in a released Clang has written forward refs for scope/inline-at, then clearly we need to handle this in the reader. But if that's not the case, perhaps we can instead change the writer to make it impossible again. (I think I might have intentionally used dyn_cast instead of dyn_cast_or_null as an assertion in the reader.)
• Either way, you might look at a bitcode-reading profile and see if this is still something to be careful about for compile-time.
• Note: I wrote that code way back before we had lazy loading of metadata and I don't remember if/how things changed there.

[tejohnson]

In D53596#1287638, @dexonsmith wrote:

In D53596#1287633, @dexonsmith wrote:

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

Sorry, I neglected to say anything actionable.

• If any bitcode writer in a released Clang has written forward refs for scope/inline-at, then clearly we need to handle this in the reader. But if that's not the case, perhaps we can instead change the writer to make it impossible again. (I think I might have intentionally used dyn_cast instead of dyn_cast_or_null as an assertion in the reader.)

It looks like this code was already handling forward refs for the scope/inline-at, since it was calling getMDNodeFwdRefOrNull - so is it possible that changed at some point?

• Either way, you might look at a bitcode-reading profile and see if this is still something to be careful about for compile-time.
• Note: I wrote that code way back before we had lazy loading of metadata and I don't remember if/how things changed there.

For lazy loading of metadata, I don't think it matters whether it is a forward reference originally, we won't load it until we need it.

I don't think the patch changes whether it handles forward references, or am I misunderstanding how the code currently works?

[tejohnson]

In D53596#1288273, @tejohnson wrote:

In D53596#1287638, @dexonsmith wrote:

In D53596#1287633, @dexonsmith wrote:

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

Sorry, I neglected to say anything actionable.

• If any bitcode writer in a released Clang has written forward refs for scope/inline-at, then clearly we need to handle this in the reader. But if that's not the case, perhaps we can instead change the writer to make it impossible again. (I think I might have intentionally used dyn_cast instead of dyn_cast_or_null as an assertion in the reader.)

It looks like this code was already handling forward refs for the scope/inline-at, since it was calling getMDNodeFwdRefOrNull - so is it possible that changed at some point?

• Either way, you might look at a bitcode-reading profile and see if this is still something to be careful about for compile-time.
• Note: I wrote that code way back before we had lazy loading of metadata and I don't remember if/how things changed there.

For lazy loading of metadata, I don't think it matters whether it is a forward reference originally, we won't load it until we need it.

I don't think the patch changes whether it handles forward references, or am I misunderstanding how the code currently works?

Ping on this comment/question - I don't think I am changing anything here on the handling of forward references in the non-lazy loading case, and I believe the need to handle them as "forward refs" is inherent in the lazy loading.

[dexonsmith]

Sorry, I missed your questions. Thanks for pinging.

I wasn’t concerned about this patch going in. I was just concerned we may have had compile time regressions already. Metadata forward references are quite expensive to track and resolve.

But I think I’d misread the patch entirely. Is see this is just loosening to Metadata .

• Am I correct that somehow the scope field is not an MDNode? The verifier should fail for that, unless something has changed. @aprantl can you take a quick look?
• Can we have a positive test for what the debug info should like like here, rather than just a crash test?

[dexonsmith]

In D53596#1298787, @dexonsmith wrote:

Sorry, I missed your questions. Thanks for pinging.

I wasn’t concerned about this patch going in. I was just concerned we may have had compile time regressions already. Metadata forward references are quite expensive to track and resolve.

But I think I’d misread the patch entirely. Is see this is just loosening to Metadata .

I misread the code in a different way. I don’t have time this week to really understand this so I’m resigning as a reviewer. I don’t want to hold up the fix and Steven already reviewed this.

• Am I correct that somehow the scope field is not an MDNode? The verifier should fail for that, unless something has changed. @aprantl can you take a quick look?
• Can we have a positive test for what the debug info should like like here, rather than just a crash test?

I’m still a bit concerned about the test being debug info related and not testing the debug info output at all, but if @aprantl has comments there they can probably be dealt with post-commit.

[steven_wu]

My thought is that this patch should make bitcode reader more robust when lazy loading, which is always a good thing. If this is a performance regression, the regression is coming from how debug information is generated. If there are no such forward-ref in the metadata, there is no slowdown with this patch. We can investigate performance regression post commit if needed.

[tejohnson]

In D53596#1298826, @steven_wu wrote:

My thought is that this patch should make bitcode reader more robust when lazy loading, which is always a good thing. If this is a performance regression, the regression is coming from how debug information is generated. If there are no such forward-ref in the metadata, there is no slowdown with this patch. We can investigate performance regression post commit if needed.

Right and note that if we are not doing lazy metadata loading, there is essentially no change here. Before, the code called MDLoader->getMDNodeFwdRefOrNull(), which boils down to dyn_cast_or_null<MDNode>(getMetadataFwdRef(Idx)). With this change we instead call MDLoader->getMetadataFwdRefOrLoad, and if there is no lazy loading enabled this will end up returning getMetadataFwdRef(ID), to which the caller is now applying the dyn_cast_or_null<MDNode>.

Steven - you had a suggestion around callers to getMDNodeFwdRefOrNull, but after this change there will be no more callers to that. So should I go ahead and submit this one, then remove that interface completely in a follow up? Or do it in this patch?

[steven_wu]

Steven - you had a suggestion around callers to getMDNodeFwdRefOrNull, but after this change there will be no more callers to that. So should I go ahead and submit this one, then remove that interface completely in a follow up? Or do it in this patch?

Doesn't matter. Follow up is perfectly fine.

[tejohnson]

In D53596#1298860, @steven_wu wrote:

Steven - you had a suggestion around callers to getMDNodeFwdRefOrNull, but after this change there will be no more callers to that. So should I go ahead and submit this one, then remove that interface completely in a follow up? Or do it in this patch?

Doesn't matter. Follow up is perfectly fine.

Ok, retesting this one at HEAD, then will remove that interface in a follow on NFC change.

[tejohnson]

My commit message lost the phabricator link, so this didn't get auto-closed. Closed by commit r346891.

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::FUNC_CODE_DEBUG_LOC: {      // DEBUG_LOC: [line, col, scope, ia]
         return error("Invalid record");
 
       unsigned Line = Record[0], Col = Record[1];
       unsigned ScopeID = Record[2], IAID = Record[3];
       bool isImplicitCode = Record.size() == 5 && Record[4];
 
       MDNode *Scope = nullptr, *IA = nullptr;
       if (ScopeID) {
-        Scope = MDLoader->getMDNodeFwdRefOrNull(ScopeID - 1);
+        Scope = dyn_cast_or_null<MDNode>(
+            MDLoader->getMetadataFwdRefOrLoad(ScopeID - 1));
         if (!Scope)
           return error("Invalid record");
       }
       if (IAID) {
-        IA = MDLoader->getMDNodeFwdRefOrNull(IAID - 1);
+        IA = dyn_cast_or_null<MDNode>(
+            MDLoader->getMetadataFwdRefOrLoad(IAID - 1));
         if (!IA)
           return error("Invalid record");
       }
       LastLoc = DebugLoc::get(Line, Col, Scope, IA, isImplicitCode);
       I->setDebugLoc(LastLoc);
       I = nullptr;
       continue;
     }

llvm/trunk/test/ThinLTO/X86/Inputs/pr35472.ll

+; ModuleID = 'b.cpp'
+source_filename = "b.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline optnone uwtable
+define void @_Z5Alphav() {
+entry:
+  call void @_Z5Bravov()
+  ret void
+}
+
+declare void @_Z5Bravov()

llvm/trunk/test/ThinLTO/X86/pr35472.ll

+; Test to make sure that lazily loaded debug location scope metadata is
+; handled properly. Note that we need to have the DILexicalScope !34
+; referenced from multiple function's debug locs for this to be in the
+; lazily loaded module level metadata block.
+
+; RUN: opt -module-hash -module-summary %s -o %t1.bc
+; RUN: opt -module-hash -module-summary %p/Inputs/pr35472.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=run %t1.bc %t2.bc
+; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOa
+; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOb
+
+; ThinLTOa-DAG: T _Z5Bravov
+; ThinLTOa-DAG: W _ZN4EchoD2Ev
+; ThinLTOb-DAG: T _Z5Alphav
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.Delta = type { %struct.Charlie }
+%struct.Charlie = type { i32 }
+%struct.Echo = type { %struct.Charlie }
+
+$_ZN4EchoD2Ev = comdat any
+$_ZN5DeltaD2Ev = comdat any
+
+define void @_Z5Bravov() !dbg !7 {
+  %Hotel = alloca %struct.Delta, align 4
+  %India = alloca %struct.Echo, align 4
+  call void @llvm.dbg.declare(metadata %struct.Delta* %Hotel, metadata !10, metadata !DIExpression()), !dbg !22
+  call void @_ZN4EchoD2Ev(%struct.Echo* %India), !dbg !28
+  ret void, !dbg !28
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+define linkonce_odr void @_ZN4EchoD2Ev(%struct.Echo* %this) unnamed_addr comdat align 2 {
+  %this.addr.i = alloca %struct.Charlie*, align 8
+  call void @llvm.dbg.declare(metadata %struct.Charlie** %this.addr.i, metadata !29, metadata !DIExpression()), !dbg !32
+  %this1.i = load %struct.Charlie*, %struct.Charlie** %this.addr.i, align 8
+  %Golf.i = getelementptr inbounds %struct.Charlie, %struct.Charlie* %this1.i, i32 0, i32 0, !dbg !33
+  ret void
+}
+
+define linkonce_odr void @_ZN5DeltaD2Ev(%struct.Delta* %this) unnamed_addr comdat align 2 !dbg !36 {
+  %this.addr.i = alloca %struct.Charlie*, align 8
+  call void @llvm.dbg.declare(metadata %struct.Charlie** %this.addr.i, metadata !29, metadata !DIExpression()), !dbg !41
+  %this1.i = load %struct.Charlie*, %struct.Charlie** %this.addr.i, align 8
+  %Golf.i = getelementptr inbounds %struct.Charlie, %struct.Charlie* %this1.i, i32 0, i32 0, !dbg !48
+  ret void
+}
+
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 321056)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "a.cpp", directory: "/home/sunil/185335/302")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "Bravo", linkageName: "_Z5Bravov", scope: !1, file: !1, line: 17, type: !8, isLocal: false, isDefinition: true, scopeLine: 17, flags: DIFlagPrototyped, isOptimized: false, unit: !0)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !DILocalVariable(name: "Hotel", scope: !7, file: !1, line: 18, type: !11)
+!11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Delta", file: !1, line: 6, size: 32, elements: !12, identifier: "_ZTS5Delta")
+!12 = !{!13}
+!13 = !DIDerivedType(tag: DW_TAG_member, name: "Foxtrot", scope: !11, file: !1, line: 7, baseType: !14, size: 32)
+!14 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Charlie", file: !1, line: 1, size: 32, elements: !15, identifier: "_ZTS7Charlie")
+!15 = !{!16, !18}
+!16 = !DIDerivedType(tag: DW_TAG_member, name: "Golf", scope: !14, file: !1, line: 3, baseType: !17, size: 32)
+!17 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!18 = !DISubprogram(name: "~Charlie", scope: !14, file: !1, line: 2, type: !19, isLocal: false, isDefinition: false, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false)
+!19 = !DISubroutineType(types: !20)
+!20 = !{null, !21}
+!21 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !14, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!22 = !DILocation(line: 18, column: 11, scope: !7)
+!24 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Echo", file: !1, line: 10, size: 32, elements: !25, identifier: "_ZTS4Echo")
+!25 = !{!26}
+!26 = !DIDerivedType(tag: DW_TAG_member, name: "Foxtrot", scope: !24, file: !1, line: 11, baseType: !14, size: 32)
+!28 = !DILocation(line: 20, column: 1, scope: !7)
+!29 = !DILocalVariable(name: "this", arg: 1, scope: !30, type: !31, flags: DIFlagArtificial | DIFlagObjectPointer)
+!30 = distinct !DISubprogram(name: "~Charlie", linkageName: "_ZN7CharlieD2Ev", scope: !14, file: !1, line: 2, type: !19, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !0, declaration: !18)
+!31 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !14, size: 64)
+!32 = !DILocation(line: 0, scope: !30)
+!33 = !DILocation(line: 2, column: 53, scope: !34)
+!34 = distinct !DILexicalBlock(scope: !30, file: !1, line: 2, column: 51)
+!36 = distinct !DISubprogram(name: "~Delta", linkageName: "_ZN5DeltaD2Ev", scope: !11, file: !1, line: 6, type: !37, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagArtificial | DIFlagPrototyped, isOptimized: false, unit: !0, declaration: !40)
+!37 = !DISubroutineType(types: !38)
+!38 = !{null, !39}
+!39 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!40 = !DISubprogram(name: "~Delta", scope: !11, type: !37, isLocal: false, isDefinition: false, flags: DIFlagArtificial | DIFlagPrototyped, isOptimized: false)
+!41 = !DILocation(line: 0, scope: !30, inlinedAt: !42)
+!42 = distinct !DILocation(line: 6, column: 8, scope: !43)
+!43 = distinct !DILexicalBlock(scope: !36, file: !1, line: 6, column: 8)
+!48 = !DILocation(line: 2, column: 53, scope: !34, inlinedAt: !42)
+
+;----------------------------------------------------------------------------------------------
+; Compiled from following two source files with 'clang++ -S --std=c++11 -O0 -g -flto=thin' 
+; struct Charlie {
+;     __attribute__((__always_inline__)) ~Charlie() { Golf = 0; }
+;     int Golf;
+; };
+; 
+; struct Delta {
+;     Charlie Foxtrot;
+; };
+; 
+; struct Echo {
+;     Charlie Foxtrot;
+;     __attribute__((nodebug)) ~Echo() = default;
+; };
+; 
+; extern void Bravo();
+; 
+; void Bravo() {
+;     Delta Hotel;
+;     Echo India;
+; }
+; -----------------------------
+; extern void Bravo();
+; extern void Alpha();
+; void Alpha() { Bravo(); }
+