[PATCH] D18252: Drop comdats from the dst module if they are not selected

[Rafael Espíndola via llvm-commits]

> Also is it even correct to use the original linkage as-is in all cases? Should it be ExternalLinkage?

Good catch. I was expecting the verifier to catch out of comdat
references to locals, but it doesn't.

> Finally, what if this is ThinLTO and the alias was defined in the original module we are importing into? Won't we have an undef at link time? Ditto for full LTO - where would this new declaration be defined? I'm not even sure what the right behavior is in that case.


With comdats the same symbols have to be defined in the comdat group
we selected. If that is not the case, an undefined error is the
correct result. The ELF wording is:

-------------------------------------------------
A symbol table entry with STB_GLOBAL or STB_WEAK binding that is
defined relative to one of a group's sections, and that is contained
in a symbol table section that is not part of the group, must be
converted to an undefined symbol (its section index must be changed to
SHN_UNDEF) if the group members are discarded. References to this
symbol table entry from outside the group are allowed.
-------------------------------------------------

> ================
> Comment at: lib/Linker/LinkModules.cpp:479
> @@ +478,3 @@
> +    GlobalValue *Declaration =
> +        new GlobalVariable(M, Ty.getElementType(), /*isConstant*/ false, L,
> +                           /*Initializer*/ nullptr);
> ----------------
> What if this was a function alias not variable alias?

That is fine. It is a bit odd that we have declaration versions of 2
of the 3 GlobalObjects (there should be only 1 IMHO), but the linker
can handle a mismatch.

> ================
> Comment at: lib/Linker/LinkModules.cpp:504
> @@ +503,3 @@
> +    bool LinkFromSource = P.second.second;
> +    if (!LinkFromSource)
> +      continue;
> ----------------
> Why not combine this into the above loop? I.e. when the ComdatsChosen array is initialized for C, you already have its LinkFromSrc. And that step is done exactly once for each comdat due to the earlier check that continues if C is already in ComdatsChosen.

Excellent suggestion. Thanks!

I will upload a new patch in a sec.