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

[Rafael Espíndola via llvm-commits]

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


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

Cheers,
Rafael