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

[Teresa Johnson via llvm-commits]

On Tue, Mar 22, 2016 at 7:17 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

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

Ok, I wasn't sure (and because this handling is for COFF not ELF). It might
be worth adding a comment to this effect where you are doing the alias
conversion to a declaration. Especially since the new test case has exactly
this situation: e.g. @alias, @func and @var etc are in one comdat but not
the selected one.

Then it sounds like normally we would expect that any alias which is
dropped would also have to be defined in the source module comdat group, so
the declaration created here would be replaced by the def from the selected
imported group from the source copy? If it isn't replaced with a def, then
if the comdat members are linkonce the defs might be dropped from the
source module when it is compiled.


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

Does "A symbol table entry ... defined relative to one of a group's
sections" refer to any alias to a member of a comdat group? I'm having a
hard time parsing this and understanding which situation it is describing,
since I would expect such an alias to be in the same symbol table section
as the rest of the group.


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

So you are saying that this will produce a declaration that the
compiler/linker is ok with, even if the aliasee is a function? That seems a
bit funky to do regardless. I see another place where we drop symbols that
creates the right kind of object type for the new declaration based on the
aliasee here: See the alias handling in  runOnModule()
in lib/Transforms/IPO/ExtractGV.cpp. Maybe that handling can be refactored
into a helper that could be invoked here...


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



-- 
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/20160322/01eb916c/attachment.html>