[ThinLTO] Fix lazy-loading of MDString instruction attachments

[mehdi_amini]

Summary

CFI is using intrinsics that takes MDString as arguments, and this
was broken during lazy-loading of metadata.

[mehdi_amini]

Nominate for LLVM 4.0 at the same time.

[pcc]

repro.zip992 KBDownload

Unfortunately this doesn't seem to entirely fix the CFI issue. With this patch applied I get these assertion failures for the attached test case (llvm-lto2 @resolution.txt -o /tmp/foo):

invalid local scope
!18644 = distinct !DILexicalBlock(scope: null, file: !5600, line: 137, column: 2)
invalid local scope
!18685 = distinct !DILexicalBlock(scope: null, file: !5600, line: 164, column: 2)
llvm-lto2: [...]/include/llvm/Support/Casting.h:95: static bool llvm::isa_impl_cl<llvm::DILocalScope, const llvm::Metadata *>::doit(const From *) [To = llvm::DILocalScope, From = const llvm::Metadata *]: Assertion `Val && "isa<> used on a null pointer"' failed.
Aborted (core dumped)

If I pass -disable-ondemand-mds-loading or if I revert this patch I get the "expected" behaviour ("expected" because of incomplete support for CFI on trunk):

Can't get register for value!
UNREACHABLE executed at [...]/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1202!

[pcc]

repro.zip985 KBDownload

Here's a reproducer that does not use CFI.

[pcc]

repro.zip1 KBDownload

Reduced reproducer.

To reproduce: PATH=/path/to/llvm/build/bin ./run-lto2.sh

[mehdi_amini]

Fix the test case reported by pcc: obviously adding a PlaceholderQueue requires to flush it at some point...

I'll add an assert in the PlaceholderQueue destructor in a separate commit.

[mehdi_amini]

Assertions added in r292597.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

502 ↗

(On Diff #85110)

This was missing ^^

[pcc]

Thanks, I was able to build chrome successfully with CFI + debug info with this patch applied. LGTM (with nits) and approved for 4.0.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

488 ↗

(On Diff #85110)

Is this function used any more?

1746 ↗

(On Diff #85110)

Should this function be renamed to match?

[mehdi_amini]

Address pcc feedback, update patch before commiting

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

private:
 
   Value *getFnValueByID(unsigned ID, Type *Ty) {
     if (Ty && Ty->isMetadataTy())
       return MetadataAsValue::get(Ty->getContext(), getFnMetadataByID(ID));
     return ValueList.getValueFwdRef(ID, Ty);
   }
 
   Metadata *getFnMetadataByID(unsigned ID) {
-    return MDLoader->getMetadataFwdRef(ID);
+    return MDLoader->getMetadataFwdRefOrLoad(ID);
   }
 
   BasicBlock *getBasicBlock(unsigned ID) const {
     if (ID >= FunctionBBs.size()) return nullptr; // Invalid ID
     return FunctionBBs[ID];
   }
 
   AttributeSet getAttributes(unsigned i) const {

llvm/trunk/lib/Bitcode/Reader/MetadataLoader.h

public:
   /// Return true if the Loader is stripping TBAA metadata.
   bool isStrippingTBAA();
 
   // Return true there are remaining unresolved forward references.
   bool hasFwdRefs() const;
 
   /// Return the given metadata, creating a replaceable forward reference if
   /// necessary.
-  Metadata *getMetadataFwdRef(unsigned Idx);
+  Metadata *getMetadataFwdRefOrLoad(unsigned Idx);
 
   MDNode *getMDNodeFwdRefOrNull(unsigned Idx);
 
   /// Return the DISubprogra metadata for a Function if any, null otherwise.
   DISubprogram *lookupSubprogramForFunction(Function *F);
 
   /// Parse a `METADATA_ATTACHMENT` block for a function.
   Error parseMetadataAttachment(

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

MetadataLoaderImpl(BitstreamCursor &Stream, Module &TheModule,
                      bool IsImporting)
       : 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(); }
-  Metadata *getMetadataFwdRef(unsigned Idx) {
-    return MetadataList.getMetadataFwdRef(Idx);
+
+  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())) {
+      PlaceholderQueue Placeholders;
+      lazyLoadOneMetadata(ID, Placeholders);
+      resolveForwardRefsAndPlaceholders(Placeholders);
+      return MetadataList.lookup(ID);
+    }
+    return MetadataList.getMetadataFwdRef(ID);
   }
 
   MDNode *getMDNodeFwdRefOrNull(unsigned Idx) {
     return MetadataList.getMDNodeFwdRefOrNull(Idx);
   }
 
   DISubprogram *lookupSubprogramForFunction(Function *F) {
     return FunctionsWithSPs.lookup(F);

 Error MetadataLoader::parseMetadata(bool ModuleLevel) {
   return Pimpl->parseMetadata(ModuleLevel);
 }
 
 bool MetadataLoader::hasFwdRefs() const { return Pimpl->hasFwdRefs(); }
 
 /// Return the given metadata, creating a replaceable forward reference if
 /// necessary.
-Metadata *MetadataLoader::getMetadataFwdRef(unsigned Idx) {
-  return Pimpl->getMetadataFwdRef(Idx);
+Metadata *MetadataLoader::getMetadataFwdRefOrLoad(unsigned Idx) {
+  return Pimpl->getMetadataFwdRefOrLoad(Idx);
 }
 
 MDNode *MetadataLoader::getMDNodeFwdRefOrNull(unsigned Idx) {
   return Pimpl->getMDNodeFwdRefOrNull(Idx);
 }
 
 DISubprogram *MetadataLoader::lookupSubprogramForFunction(Function *F) {
   return Pimpl->lookupSubprogramForFunction(F);

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

 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary %s -o %t.bc -bitcode-mdindex-threshold=0
 ; RUN: opt -module-summary %p/Inputs/lazyload_metadata.ll -o %t2.bc -bitcode-mdindex-threshold=0
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
 ; REQUIRES: asserts
 
 ; Check that importing @globalfunc1 does not trigger loading all the global
 ; metadata for @globalfunc2 and @globalfunc3
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:          -o /dev/null -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=LAZY
 ; LAZY: 49 bitcode-reader  - Number of Metadata records loaded
-; LAZY: 1 bitcode-reader  - Number of MDStrings loaded
+; LAZY: 2 bitcode-reader  - Number of MDStrings loaded
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:          -o /dev/null -disable-ondemand-mds-loading -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=NOTLAZY
 ; NOTLAZY: 58 bitcode-reader  - Number of Metadata records loaded
-; NOTLAZY: 6 bitcode-reader  - Number of MDStrings loaded
+; NOTLAZY: 7 bitcode-reader  - Number of MDStrings loaded
 
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define void @globalfunc1(i32 %arg) {
+  %x = call i1 @llvm.type.test(i8* undef, metadata !"typeid1")
   %tmp = add i32 %arg, 0, !metadata !2
   ret void
 }
 
 ; We need two functions here that will both reference the same metadata.
 ; This is to force the metadata to be emitted in the global metadata block and
 ; not in the function specific metadata.
 ; These function are not imported and so we don't want to load their metadata.
 
 define void @globalfunc2(i32 %arg) {
+  %x = call i1 @llvm.type.test(i8* undef, metadata !"typeid1")
   %tmp = add i32 %arg, 0, !metadata !1
   ret void
 }
 
 define void @globalfunc3(i32 %arg) {
   %tmp = add i32 %arg, 0, !metadata !1
   ret void
 }
 
+declare i1 @llvm.type.test(i8* %ptr, metadata %bitset) nounwind readnone
+
 !1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
 !2 = !{!"Hello World"}
 !3 = !{!"3"}
 !4 = !{!"4"}
 !5 = !{!"5"}
 !6 = !{!9}
 !7 = !{!"7"}
 !8 = !{!"8"}
-!9 = !{!6}
+!9 = !{!6}
 No newline at end of file