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

[Rafael Espíndola via llvm-commits]

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

>
> ================
> 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.

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