[PATCH] D23015: [ThinLTO/gold] Conservatively handle unknown GVs when internalizing

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D23015#503013, @tejohnson wrote:

> In https://reviews.llvm.org/D23015#502970, @mehdi_amini wrote:
>
> > OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
> >  Then why not changing this?
>
>
> We use gold's symbol resolution to determine which values to link in. So in the example, it knows that f1 is a preempted symbol in %t2.o and doesn't need to include its definition. I don't think we should need to change that.


I see...
I still does not think that's a great thing: using the `IRMover` just to get a module where some symbols are pruned seems quite sketchy to me.

Here we accept that the module can have renamed symbols from what is in the index, but that should be an invariant or we'll run in trouble at some point. So I'm not convinced the fix is right at this point.

I dug a bit more, I'm still trying to follow how the bug happens: the IRMover is forcing `@f1` (in Inputs/thinlto_alias2.ll) to be linked to satisfy the alias `@a2`. And since there is already a declaration for `@f1` when it does so, it is supposed to create a local copy (i.e. it will be an *internal* function), IRMover.cpp ~line 930.

Now the code you are patching is a callback for the internalize utility, and IIUC it is not called on internal function (which I expect to be the case here), as I read `InternalizePass::shouldPreserveGV`.


https://reviews.llvm.org/D23015