[PATCH] D18390: Fix logic for which symbols to keep with comdats

[Teresa Johnson via llvm-commits]

On Wed, Mar 23, 2016 at 1:15 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 23 March 2016 at 16:06, Teresa Johnson <tejohnson at google.com> wrote:
> > tejohnson added a comment.
> >
> > I'm not sure how these changes are fixing the issue described - we used
> to always add to ValuesToLink any GV in a chosen comdat, but here we only
> sometimes do. Can't that result in incomplete comdat groups?
>
> No. The idea is that dropping a comdat will drop a symbol. Not
> dropping it just means it goes to normal symbol resolution, not that
> it *must* be included.
>
> So, if we don't drop a comdat, we will still look at every symbol in
> that comdat and not get an incomplete group. It is possible that we
> will decide to use an existing symbol instead, but that is fine.
>

I see, if we go to the shouldLinkFromSource then we had an existing symbol,
so we are guaranteed to get a copy of each symbol and therefore a complete
group.


> > I tried the new test case with HEAD and see that it doesn't have a $c1
> comdat at all, but not sure why or how that is fixed by the patch. I'm
> obviously missing something, can you give me a better idea of how this is
> working?
>
> At head we force the IRMover to keep (for example) the @v1 from
> Inputs/comdat16.ll. Since these symbols have the c2 comdat, the c1
> comdat in the result is empty and is dropped.
>

Ok, got it.


>
> >
> > ================
> > Comment at: lib/Linker/LinkModules.cpp:425
> > @@ -424,4 +424,3 @@
> >      std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
> > -    if (LinkFromSrc)
> > -      ValuesToLink.insert(&GV);
> > -    return false;
> > +    if (!LinkFromSrc)
> > +      return false;
> > ----------------
> > We now only add comdat members to ValuesToLink on a case by case basis
> below.
>
> Yes. That is the main part of the patch. A comdat can drop parts of a
> file, it doesn't force the symbol resolution of what is left.
>
> > ================
> > Comment at: lib/Linker/LinkModules.cpp:567
> > @@ -567,2 +566,3 @@
> >      for (GlobalValue *GV2 : ComdatMembers[SC])
> > -      ValuesToLink.insert(GV2);
> > +      if (GV2->hasInternalLinkage())
> > +        ValuesToLink.insert(GV2);
> > ----------------
> > Why only internal members? If we have a comdat member in ValuesToLink
> (added in linkIfNeeded), then presumably we had selected that copy of the
> comdat and wouldn't we want all members?
>
> The correct thing is to drop this entire loop. I indent to do that in
> a followup patch.
>

I see now why we need to add internal members to the ValuesToLink set here
- when we selected the dest copy of that particular symbol but need to keep
a local (will be renamed) copy of the source symbol for references, right?


> >
> > ================
> > Comment at: test/Linker/Inputs/comdat16.ll:5
> > @@ +4,3 @@
> > +; This is only present in this file. The linker will keep $c1 from the
> first
> > +; file and this will be undefined.
> > + at will_be_undefined = global i32 1, comdat($c1)
> > ----------------
> > Should test check that?
>
> I will add that.
>
> Thanks,
> Rafael
>



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160323/83c0ceae/attachment.html>