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

[Teresa Johnson via llvm-commits]

On Fri, Jan 12, 2018 at 9:24 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Teresa Johnson via Phabricator <reviews at reviews.llvm.org> writes:
>
> > 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?
>
> It is not kept. In the regular LTO case a non prevailing symbol is
>

Ok I see what happens. In addRegularLTO only prevailing symbols are added
to the Keep set used when linking the IR (and a few other cases like
linkonce_odr). In ThinLTO, obviously we don't have the IR during the thin
link.

Here's what we currently do with the prevailing info for ThinLTO:
- in addThinLTO we already keep track of the prevailing module for a symbol
(PrevailingModuleForGUID)
- this is only used right now in weak symbol resolution - for linkonce, we
upgrade the prevailing symbol to weak, and for other WeakForLinker symbols
we downgrade the non-prevailing to available_externally (when legal)

What we probably can/should do: Use PrevailingModuleForGUID to:
1) prevent importing of the non-prevailing copy when invoking
ComputeCrossModuleImport
2) force the non-prevailing symbols to be internalized
via thinLTOInternalizeAndPromoteInIndex

For #2, we can assert in isExported that anything in an ExportList (because
it was imported) is the prevailing copy (after doing #1). Otherwise, if the
symbol is not prevailing in that module then return false from isExported
to ensure it is internalized.

Does that sound right?


dropped.
>
> Cheers,
> Rafael
>



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
<(408)%20460-2413>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180112/f8c225c8/attachment.html>