[ThinLTO] Fix crash when importing an opaque type

[mehdi_amini]

Summary

It seems that because ThinLTO does not import the full module,
some invariant of the type mapper are broken.

In Monolithic LTO, we import every globals: when calling
IRLinker::copyFunctionProto() on @foo(), we end-up calling
TypeMapTy::get(FTy) on the type of @foo(), which will map
%0 and record the destination as opaque.

ThinLTO skips this because @foo is not imported and goes directly
to the next stage.

Next we call computeTypeMapping() that map the types for each
globals, and ends up checking for type isomorphism, and may add
type mapping. However it didn't record if there was an opaque
destination type that was resolved, which is what this patch
addresses.

[mehdi_amini]

Teresa: I'd like to backport this in 3.9, but this is only possible if we land this *now*, so please prioritize appropriately!

[tejohnson]

ThinLTO skips this because @foo is not imported and goes directly
to the next stage.

I see - because @foo is in the importing module, so it isn't IR linked in like in the monolithic case where all modules are linked in, right?

A couple questions:

• I see that the DstStructTypesSet in the IRLinker's TypeMap is initialized from the IdentifiedStructTypes built from the types in the original (importing) module in the IRMover constructor. This adds all the struct types to either the Opaque or NonOpaque sets. Why isn't the opaque type in the original importing (Dst) module being added appropriately there?
• What about adding to the NonOpaque set? If we're missing cases of adding opaque types are we similarly missing adding non-opaque types?

llvm/test/ThinLTO/X86/Inputs/import_opaque_type.ll

16 ↗

(On Diff #78477)

Add newline

llvm/test/ThinLTO/X86/import_opaque_type.ll

28 ↗

(On Diff #78477)

Add newline

[mehdi_amini]

In D26840#599745, @tejohnson wrote:

ThinLTO skips this because @foo is not imported and goes directly
to the next stage.

I see - because @foo is in the importing module, so it isn't IR linked in like in the monolithic case where all modules are linked in, right?

A couple questions:

• I see that the DstStructTypesSet in the IRLinker's TypeMap is initialized from the IdentifiedStructTypes built from the types in the original (importing) module in the IRMover constructor. This adds all the struct types to either the Opaque or NonOpaque sets. Why isn't the opaque type in the original importing (Dst) module being added appropriately there?

Opaque type are not recognized as struct, this is dead code: addOpaque can't be called from here I believe.

• What about adding to the NonOpaque set? If we're missing cases of adding opaque types are we similarly missing adding non-opaque types?

I'll look into it.

[tejohnson]

In D26840#599810, @mehdi_amini wrote:

In D26840#599745, @tejohnson wrote:

• I see that the DstStructTypesSet in the IRLinker's TypeMap is initialized from the IdentifiedStructTypes built from the types in the original (importing) module in the IRMover constructor. This adds all the struct types to either the Opaque or NonOpaque sets. Why isn't the opaque type in the original importing (Dst) module being added appropriately there?

Opaque type are not recognized as struct, this is dead code: addOpaque can't be called from here I believe.]

But the block of code you modified is only entered if we identify the dest type as a struct type:

if (cast<StructType>(DstTy)->isOpaque()) {

[mehdi_amini]

In D26840#599810, @mehdi_amini wrote:

In D26840#599745, @tejohnson wrote:

ThinLTO skips this because @foo is not imported and goes directly
to the next stage.

I see - because @foo is in the importing module, so it isn't IR linked in like in the monolithic case where all modules are linked in, right?

A couple questions:

• I see that the DstStructTypesSet in the IRLinker's TypeMap is initialized from the IdentifiedStructTypes built from the types in the original (importing) module in the IRMover constructor. This adds all the struct types to either the Opaque or NonOpaque sets. Why isn't the opaque type in the original importing (Dst) module being added appropriately there?

Opaque type are not recognized as struct, this is dead code: addOpaque can't be called from here I believe.

• What about adding to the NonOpaque set? If we're missing cases of adding opaque types are we similarly missing adding non-opaque types?

I'll look into it.

The code you identified correctly catch all the non-opaque struct!

[tejohnson]

In D26840#599846, @mehdi_amini wrote:

In D26840#599810, @mehdi_amini wrote:

In D26840#599745, @tejohnson wrote:

ThinLTO skips this because @foo is not imported and goes directly
to the next stage.

I see - because @foo is in the importing module, so it isn't IR linked in like in the monolithic case where all modules are linked in, right?

A couple questions:

• I see that the DstStructTypesSet in the IRLinker's TypeMap is initialized from the IdentifiedStructTypes built from the types in the original (importing) module in the IRMover constructor. This adds all the struct types to either the Opaque or NonOpaque sets. Why isn't the opaque type in the original importing (Dst) module being added appropriately there?

Opaque type are not recognized as struct, this is dead code: addOpaque can't be called from here I believe.

• What about adding to the NonOpaque set? If we're missing cases of adding opaque types are we similarly missing adding non-opaque types?

I'll look into it.

The code you identified correctly catch all the non-opaque struct!

Ok, so then the question is how that is not catching the opaque type: see my response to your first response:

But the block of code you modified is only entered if we identify the dest type as a struct type:

if (cast<StructType>(DstTy)->isOpaque()) {

So at that point we are detecting the dest opaque type as a StructType, so why wouldn't we catch those here too?

[mehdi_amini]

Add newlines at the end of the file

[mehdi_amini]

But the block of code you modified is only entered if we identify the dest type as a struct type:

if (cast<StructType>(DstTy)->isOpaque()) {

So at that point we are detecting the dest opaque type as a StructType, so why wouldn't we catch those here too?

Sorry I was wrong in my first walkthrough, the issue is that we initialize the type finder with:

StructTypes.run(M, true);

The "true" means "OnlyNamed", this is why we don't have it. I have no idea why it only gets the named one here or what would be the impact to get them all.

[tejohnson]

In D26840#599898, @mehdi_amini wrote:

But the block of code you modified is only entered if we identify the dest type as a struct type:

if (cast<StructType>(DstTy)->isOpaque()) {

So at that point we are detecting the dest opaque type as a StructType, so why wouldn't we catch those here too?

Sorry I was wrong in my first walkthrough, the issue is that we initialize the type finder with:

StructTypes.run(M, true);

The "true" means "OnlyNamed", this is why we don't have it. I have no idea why it only gets the named one here or what would be the impact to get them all.

Tracked it down to this commit from 2012:

---

commit d46575f1908b1fb9950e610a1f36733893ad44b1
Author: Bill Wendling <[e-mail]>
Date: Sat Apr 21 23:59:16 2012 +0000

Add a flag to the struct type finder to collect only those types which have
names. This saves collecting types we normally don't care about.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@155300 91177308-0d34-0410-b5e6-96231b3b80d8

---

Looks like it was an optimization. I think the right place to fix it is here - just collect all struct types. The current patch seems like a bandaid for the fact that we aren't doing this up front here.

[mehdi_amini]

The thing that makes me worried is that this only about the initial types: I wonder if you couldn't start with an empty module, and then import an opaque type, and only then import the definition for this type. I'll try to forge an example.

Also there are calls to addOpaque() during the import, what you suggest seems that some cleanup would be needed.

[tejohnson]

In D26840#599984, @mehdi_amini wrote:

The thing that makes me worried is that this only about the initial types: I wonder if you couldn't start with an empty module, and then import an opaque type, and only then import the definition for this type. I'll try to forge an example.

Also there are calls to addOpaque() during the import, what you suggest seems that some cleanup would be needed.

I thought the issue here was for the dest module, which isn't being imported, and is why the constructor is walking its types to initialize the DstStructTypesSet. The other call to addOpaque is when the source type is opaque, which is the importing case. switchToNonOpaque seems to handle the other case you mention (have an opaque type in dest module, then import a def).

[mehdi_amini]

Map all types upfront!

[mehdi_amini]

I checked on another example that this should be enough! Opaque types that are imported will be mapped appropriately at the time of the import.

[tejohnson]

Great, thanks. LGTM!

(Note description is now out of date, just make sure it is correct in the commit log message)

llvm/trunk/lib/Linker/IRMover.cpp

bool IRMover::IdentifiedStructTypeSet::hasType(StructType *Ty) {
   if (Ty->isOpaque())
     return OpaqueStructTypes.count(Ty);
   auto I = NonOpaqueStructTypes.find(Ty);
   return I == NonOpaqueStructTypes.end() ? false : *I == Ty;
 }
 
 IRMover::IRMover(Module &M) : Composite(M) {
   TypeFinder StructTypes;
-  StructTypes.run(M, true);
+  StructTypes.run(M, /* OnlyNamed */ false);
   for (StructType *Ty : StructTypes) {
     if (Ty->isOpaque())
       IdentifiedStructTypes.addOpaque(Ty);
     else
       IdentifiedStructTypes.addNonOpaque(Ty);
   }
   // Self-map metadatas in the destination module. This is needed when
   // DebugTypeODRUniquing is enabled on the LLVMContext, since metadata in the

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

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+%0 = type { i8 }
+
+%a = type { %0 * }
+
+define void @bar(%a *) {
+  ret void
+}
+
+define void @baz() {
+  call void @bar(%a *null)
+  ret void
+}

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

+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/import_opaque_type.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+
+; Check that we import correctly the imported type to replace the opaque one here
+; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s
+
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+; CHECK: %0 = type { i8 }
+%0 = type opaque
+
+%a = type { %0 * }
+
+declare void @baz()
+define void @foo(%a *) {
+  call void @baz()
+  ret void
+}
+
+define i32 @main() {
+    call void @foo(%a *null)
+  ret i32 0
+}