[PATCH] D41988: [ThinLTO] - Set WeakAnyLinkage for all LinkerRedefined symbols.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D41988#974637, @grimar wrote:

> In https://reviews.llvm.org/D41988#974604, @tejohnson wrote:
>
> > The same handling occurs for regular LTO (setting LinkerRedefined to WeakAny only when prevailing). Is this a problem there? If not, why?
>
>
> Yeah, I saw the same code in regular LTO, 
>  but it is not a problem for it, our defsym.ll testcase mentioned has both ThinLTO and regular LTO sub-cases and that problem occurs only for ThinLTO case.
>
> > Answering my own question perhaps, but I guess this is because in the ThinLTO case we end up importing bar2 and therefore promoting it to external linkage instead of leaving it alone which would cause it to be internalized?
>
> That part I believe is exactly what I observed during debugging of ThinLTO.


Normally we would have a prevailing copy that would be imported, but I guess there is none here. Thinking more about the regular LTO case, since there also isn't a prevailing copy of bar2, is that non-prevailing copy kept in the merged module, and if so, wouldn't it be incorrectly inlined because it won't be set to WeakAnyLinkage?

> Importing happens here:
>  (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L181)
> 
> With WeakAnyLinkage `isInterposableLinkage` returns false and then exporting from their source module does not happen:
>  https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L274
> 
> What result in InternalLinkage set here
>  https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L337
> 
>> It seems like the bug then is that we import a non-prevailing value. Although the best fix for that in general seems to be what this patch is doing - marking as WeakAnyLinkage.




https://reviews.llvm.org/D41988