[rfc][gold plugin] Fix pr19901

[Rafael Espíndola]

>> I would call it a missing feature. It is not as bad as assuming a
>> whole program. What the the current design of lib/Linker assumes is
>> that each module in "independent" given the IR semantics. By
>> independent I mean things like "no undefined reference to a
>> linkonce_odr", which follows from the rule than an unused linkonce_odr
>> can be dropped.
>
>
> *sigh*
>
> The LangRef doesn't describe what it means by "can be dropped if not used".
> Can we drop them in codegen if they aren't used? What if we split up the
> module into per-function pieces (say, for parallel codegen) then try to
> native link the resulting .o files? Now splitting up a module is not a safe
> operation, which in turn means that of course joining them isn't either?

I guess you can say so, yes. Every code along the path would have to
learn about a linkonce_odr being required or that GV would have to be
patched to be weak_odr while it passes code that uses plain IR
semantics.

> If it were going to be "only used within this TU" then it's internal, right?

Almost. There is pointer equality to be taken into consideration. But
yes, a linkonce_odr whose address is not significant can be made
internal. It is just that it is not profitable to do it for each .o as
it introduces too much code duplication. We do it at LTO time
(LDPR_PREVAILING_DEF_IRONLY_EXP) and ld64 can do it during regular
linking (WeakDefAutoPrivate).

> So then linkonce_odr means "only used within this whole program"?

I don't think so. For 99% of the code the "in this module" is the
semantics we want. I mean, we do use the fact that it can be hidden
inside a DSO.

> Or "only
> used within this shared object"? Or what? Where's the point at which we can
> drop it?

Can? Anytime we want, as long as the address is not significant. It
might just not be profitable because of the code duplication.

Side note: I did try in the past to implement an optimization that
would give hidden visibility to a "linkonce_odr unnamed_addr". The
problem we get into is that ELF mandates that when combining symbols
with different visibilites the most restrictive one wins, so we would
always hide it during linking, breaking pointer equality.

> Or is it "only used within this TU, but multiple definitions aren't
> supposed to be an error"? That can't be right because then we wouldn't have
> references coming out of native ELF, right?

Normally there isn't. The intermediate stage in LTO is that is
special. The linker (bfd/gold) selects a symbol form a particular .o,
and the rest becomes undefined references. That is why the plugin is
in a special situation.

> If we already drop unused linkonce_odr functions in codegen, then I'll
> accept that your approach is correct. Otherwise I'm sure it should be a
> change in the linker.

I don't think we drop them yet, but I see it is as just a convenience
so that it is easy to test it with llc. While developing this patch I
did a few clang bootstraps and only once noticed a unused linkonce_odr
in a .bc produced by clang. The previous passes should drop it. We
could add an option to do it, I am just not sure it would have a big
impact (I will write a patch to check).

In summary, the plugin is in a special situation with regards to a few
things, the main one being undefined references to linkonce_odr. It
would definitely benefit from a more flexible lib/Linker interface to
avoid the dance this patch does. I intend to try to implement that
interface, but would like to do it after the immediate goals of fixing
pr19901 and having the plugin support COMDATs.

Cheers,
Rafael