[PATCH] D26840: [ThinLTO] Fix crash when importing an opaque type

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26840#599846, @mehdi_amini wrote:

> In https://reviews.llvm.org/D26840#599810, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/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?


https://reviews.llvm.org/D26840