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

[Teresa Johnson via llvm-commits]

On Tue, Mar 22, 2016 at 12:52 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> >
> > 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.
> >
>
> Note that this is not a new problem. The patch just makes us handle
> the case of the files being seen in the opposite order. If currently
> you run
>
> llvm-link a.ll b.ll -o c.ll
>
> and we pick a group from a, it is possible that you will end up with
> an undefined reference from something being dropped from b.ll. This
> patch just implements us selecting the group from b.ll (which only
> happens in COFF).
>

Ok, applied this patch and modified the test case to have the same comdat
members and aliases in second module (with largest) and confirmed that the
defs do get pulled in when linked as expected (aliases end up as aliases
not decls). So that looks good to me. What is the remaining problem?


>
> >> -------------------------------------------------
> >> 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?
>
> It can. At the ELF level there are no aliases. Just symbols that
> happen to have the same value.
>
> > So you are saying that this will produce a declaration that the
> compiler/linker is ok with, even if the aliasee is a function?
>
> Correct.
>
> > 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...
>
> I don't think it is that funky, it is inherent on llvm being type
> unsafe, which is why I would prefer to be just one way to declare an
> undefined symbol in the IR.
>
> But in any case. It is a simple case. I will upload a new patch with
> it. Adding the test for it I also noticed that aliases had to be
> processed first.
>

Ok, thanks. This just looks cleaner to me. Would be nice to have a single
generic way to create a declaration as you note.

Teresa


> Cheers,
> 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/20160322/288369c7/attachment.html>