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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In https://reviews.llvm.org/D41988#974645, @tejohnson wrote:

> 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?


>From what I see it is not inlined in output: https://github.com/llvm-mirror/lld/blob/master/test/ELF/lto/defsym.ll#L17 (regular LTO test passes and result looks expected),
but let me debug it on moday. Honestly I did not look into regular LTO code very deeply as it just worked for us.
I'll return with more details. Thanks for looking at this !


https://reviews.llvm.org/D41988