Fix a crash in lazy loading of Metadata in ThinLTO

[Sunil_Srivastava]

Summary

During the lazy loading pass of ThinLTO, if the load of a function body
creates forward references, resolve them right there.

This fixes PR35472.

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

4620

I think this is just hiding the issue that the below error is trying to catch. That is - we created a forward ref and didn't resolve it as expected during the function parsing.

What created the forward ref? Do you know why it wasn't resolved in the usual way?

The description of r291027 lists some limitations - is there another one that needs to be handled specially (e.g. like the METADATA_GLOBAL_DECL_ATTACHMENT case)?

[mehdi_amini]

test/ThinLTO/X86/pr35472-a.ll

133

Can you reduce the test case? And make it very clear *what* is the exact structure that lead to the issue?

See the kind of test here: llvm/trunk/test/ThinLTO/X86/lazyload_metadata.ll

[Sunil_Srivastava]

What created the forward ref? Do you know why it wasn't resolved in the usual way?

What is the usual way of resolving forward refs?

I modeled FinishFunctionMetadata code from ..MetadataLoaderImpl::parseMetadats just below it, which calls lazyLoadModuleMetadataBlcok and then resolveForwardRefsandPlaceholders just after that. So I assumed that will be the usual way.

The unique thing in this test case is that a normal function, with debug info, gets inlined inside a nodebug function. The inlined instructions have attached metadata whereas the whole function is still nodebug.

The semantics of this situation is not clearly defined. We could rule that the debug info from inlined code should be stripped., which will avoid this problem in the first case, but I can see that some users may carefully choose where they want debug info and the inlining will foil their attempt then.

[Sunil_Srivastava]

I have trimmed the .ll test somewhat, but I am not sure what is the exact structure that is leading to this error.

The error seems to be related to the limitation (2) mentioned in r291027 for function level, and the fix is along the lines of MetadataLoaderImpl::parseMetadats for the global level, i.e, call resolveForwardRefsAndPlaceholders().

I will be happy to hear any suggestion about where to look for more info on this mechanism and how to proceed.

[Sunil_Srivastava]

The thing that I do not understand is that, even though the error happens while reading EchoD2, a change in DeltaD2 avoid the error.

Specifically, removing the getelementptr instr from DeltaD2 avoids the error, even though the error in the failing run occurs before reading DeltaD2.

Under debugger readers of these two differing version follow widely different paths.

Are there any kind of internal documentation of notes describing what lazy loading is doing precisely?

test/ThinLTO/X86/pr35472-a.ll

133

There is the smallest C++ test case that I could come up with, but I have not attempted to reduce the IR. I will try that.

The exact structure leading to the error is inlining of a normal (not nodebug) function inside a nodug function. That leads to an instruction with attached debuginfo inside an otherwise nodebug function.

[dmikulin]

Any chance this gets fixed soon? I'm hitting the same problem.

[vsk]

Hi @tejohnson @Sunil_Srivastava, just a heads-up that with https://reviews.llvm.org/rL344545 applied, I'm hitting "Invalid function metadata: outgoing forward refs" in an internal project with ThinLTO and hot/cold splitting enabled. There's some sort of problem caused by having an outlined function with debug locations in it but no DISubprogram. I'll try to dig a bit deeper.

[hiraditya]

Can we land this patch

[tejohnson]

In D41474#1270284, @hiraditya wrote:

Can we land this patch

Let me take a look today. As noted in my earlier comment, this still seems like a hammer to fix something that was intended to be handled earlier by the MDLoader. I'd like to understand why it didn't work as intended.

[dexonsmith]

In D41474#1270703, @tejohnson wrote:

In D41474#1270284, @hiraditya wrote:

Can we land this patch

Let me take a look today. As noted in my earlier comment, this still seems like a hammer to fix something that was intended to be handled earlier by the MDLoader. I'd like to understand why it didn't work as intended.

FWIW I'm concerned about this patch too. I think we need to understand the issue (and as per Mehdi's comments, understand the exact structure leading to the issue) before committing a fix.

[tejohnson]

In D41474#1271879, @dexonsmith wrote:

In D41474#1270703, @tejohnson wrote:

In D41474#1270284, @hiraditya wrote:

Can we land this patch

Let me take a look today. As noted in my earlier comment, this still seems like a hammer to fix something that was intended to be handled earlier by the MDLoader. I'd like to understand why it didn't work as intended.

FWIW I'm concerned about this patch too. I think we need to understand the issue (and as per Mehdi's comments, understand the exact structure leading to the issue) before committing a fix.

Quick note that I have been looking at this and have a rough idea of what is going on and another possible fix. Needs some more investigation tomorrow.

[kristina]

Just a small nitpick, please remove the svn:executable property on test/ThinLTO/X86/pr35472-a.ll if you ever get this to a point where it's approved and committed, some buildbots get upset if it's present and will fail while updating their checkouts.

[tejohnson]

Ok here's what is going on.

The scope of the DILocation in _ZN4EchoD2Ev has a scope that is a DILexicalBlock. In the test case, because that same DILexicalBlock is used as the scope for DILocation from two different routines (_ZN4EchoD2Ev and _ZN5DeltaD2Ev), it lives in the MODULE's metadata block. Therefore, during importing, we try to load it lazily. We parse the debug location when from BitcodeReader::parseFunction, in the FUNC_CODE_DEBUG_LOC case. This case invokes MDLoader->getMDNodeFwdRefOrNull() for the scope and inlinedAt fields, which creates a forward ref if it hasn't been loaded already, which it hasn't since it lives in the lazily loaded Module level metadata.

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.

The reason the problem goes away as noted by the patch author when the getelementptr in _ZN5DeltaD2Ev is removed (actually, all it takes is removing the !dbg attachment there), is that the DILexicalBlock then lives in the function metadata block for _ZN4EchoD2Ev since it is only referenced by one function. The function metadata block as mentioned above is loaded eagerly.

I'm not sure why we haven't hit this too often historically (I put in an assert where we get the scope/inlinedAt that it isn't a temporary and it isn't hit at all for the current llvm regression suite), but I am guessing that the reason we suddenly started hitting with ThinLTO+hotcold function splitting is that more scopes are being referenced by multiple functions (part in the hot split func and part in the cold split func) and therefore moving to the lazily loaded module level metadata. This is likely also happening because splitting is/was happening in the pre-link compile step before importing (this was fixed for the old PM in D53437 but as I noted there is not yet fixed for the new PM).

The fix I tried that works both for this test case and for the large app where I hit it after enabling hot cold splitting with ThinLTO is to change the scope and inlinedAt handling to instead invoke getMetadataFwdRefOrLoad. If this seems like a reasonable fix (I am not sure why it wouldn't be), I can send a new patch with that fix. Here's the fix that worked for me:

• a/lib/Bitcode/Reader/BitcodeReader.cpp

+++ b/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -3520,12 +3520,12 @@ Error BitcodeReader::parseFunctionBody(Function *F) {

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");
}

[tejohnson]

I mailed D53596 with the new proposed fix.

[tejohnson]

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

[tejohnson]

In D41474#1287608, @tejohnson wrote:

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

Nevermind - pinged the wrong one, meant to ping follow on D53596

[Sunil_Srivastava]

Abandoned in favor of https://reviews.llvm.org/D53596

lib/Bitcode/Reader/BitcodeReader.cpp

if (!A->getParent()) {
           A->replaceAllUsesWith(UndefValue::get(A->getType()));
           delete A;
         }
       }
       return error("Never resolved value found in function");
     }
   }
 
+  // Resolve any forward refs found while loading the function.
+  if (MDLoader->hasFwdRefs())
+    MDLoader->finishFunctionMetadata();
+
   // Unexpected unresolved metadata about to be dropped.
   if (MDLoader->hasFwdRefs())
     return error("Invalid function metadata: outgoing forward refs");
 
   // Trim the value list down to the size it was before we parsed this function.
   ValueList.shrinkTo(ModuleValueListSize);
   MDLoader->shrinkTo(ModuleMDLoaderSize);
   std::vector<BasicBlock*>().swap(FunctionBBs);

lib/Bitcode/Reader/MetadataLoader.h

public:
   MetadataLoader(MetadataLoader &&);
 
   // Parse a module metadata block
   Error parseModuleMetadata() { return parseMetadata(true); }
 
   // Parse a function metadata block
   Error parseFunctionMetadata() { return parseMetadata(false); }
 
+  // resolve any forward refs at the end of the parsing of a function.
+  void finishFunctionMetadata();
+
   /// Set the mode to strip TBAA metadata on load.
   void setStripTBAA(bool StripTBAA = true);
 
   /// Return true if the Loader is stripping TBAA metadata.
   bool isStrippingTBAA();
 
   // Return true there are remaining unresolved forward references.
   bool hasFwdRefs() const;

lib/Bitcode/Reader/MetadataLoader.cpp

MetadataLoaderImpl(BitstreamCursor &Stream, Module &TheModule,
       : MetadataList(TheModule.getContext()), ValueList(ValueList),
         Stream(Stream), Context(TheModule.getContext()), TheModule(TheModule),
         getTypeByID(std::move(getTypeByID)), IsImporting(IsImporting) {}
 
   Error parseMetadata(bool ModuleLevel);
 
   bool hasFwdRefs() const { return MetadataList.hasFwdRefs(); }
 
+  void finishFunctionMetadata();
+
   Metadata *getMetadataFwdRefOrLoad(unsigned ID) {
     if (ID < MDStringRef.size())
       return lazyLoadOneMDString(ID);
     if (auto *MD = MetadataList.lookup(ID))
       return MD;
     // If lazy-loading is enabled, we try recursively to load the operand
     // instead of creating a temporary.
     if (ID < (MDStringRef.size() + GlobalMetadataBitPosIndex.size())) {
case BitstreamEntry::Record: {
         return false;
       }
       break;
     }
     }
   }
 }
 
+// After reading a function body, resolve any forward refs.
+void MetadataLoader::MetadataLoaderImpl::finishFunctionMetadata()
+{
+  PlaceholderQueue Placeholders;
+  resolveForwardRefsAndPlaceholders(Placeholders);
+  upgradeDebugInfo();
+}
+
 /// Parse a METADATA_BLOCK. If ModuleLevel is true then we are parsing
 /// module level metadata.
 Error MetadataLoader::MetadataLoaderImpl::parseMetadata(bool ModuleLevel) {
   if (!ModuleLevel && MetadataList.hasFwdRefs())
     return error("Invalid metadata: fwd refs into function blocks");
 
   // Record the entry position so that we can jump back here and efficiently
   // skip the whole block in case we lazy-load.
MetadataLoader::MetadataLoader(BitstreamCursor &Stream, Module &TheModule,
                                std::function<Type *(unsigned)> getTypeByID)
     : Pimpl(llvm::make_unique<MetadataLoaderImpl>(
           Stream, TheModule, ValueList, std::move(getTypeByID), IsImporting)) {}
 
 Error MetadataLoader::parseMetadata(bool ModuleLevel) {
   return Pimpl->parseMetadata(ModuleLevel);
 }
 
+void MetadataLoader::finishFunctionMetadata() {
+  Pimpl->finishFunctionMetadata();
+}
+
 bool MetadataLoader::hasFwdRefs() const { return Pimpl->hasFwdRefs(); }
 
 /// Return the given metadata, creating a replaceable forward reference if
 /// necessary.
 Metadata *MetadataLoader::getMetadataFwdRefOrLoad(unsigned Idx) {
   return Pimpl->getMetadataFwdRefOrLoad(Idx);
 }
 

test/ThinLTO/X86/Inputs/pr35472-b.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() #0 !dbg !7 {
+entry:
+  call void @_Z5Bravov(), !dbg !10
+  ret void, !dbg !11
+}
+
+declare void @_Z5Bravov() #1
+
+attributes #0 = { noinline optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!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: "b.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}
+!6 = !{!"clang version 6.0.0 (trunk 321056)"}
+!7 = distinct !DISubprogram(name: "Alpha", linkageName: "_Z5Alphav", scope: !1, file: !1, line: 3, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !DILocation(line: 3, column: 16, scope: !7)
+!11 = !DILocation(line: 3, column: 25, scope: !7)

test/ThinLTO/X86/pr35472-a.ll

+; RUN: opt -module-hash -module-summary %s -o %t1.bc
+; RUN: opt -module-hash -module-summary %p/Inputs/pr35472-b.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
+; ThinLTOb-DAG: U _Z5Bravov
+
+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() #0 !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) #2, !dbg !28
+  ret void, !dbg !28
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+
+define linkonce_odr void @_ZN4EchoD2Ev(%struct.Echo* %this) unnamed_addr #0 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 #0 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, variables: !2)
+!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, variables: !2)
+!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, variables: !2)
+!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(); }
+