[PATCH] D28523: [ThinLTO] Ensure objects from static libraries linked in correct order

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D28523#641676, @pcc wrote:

> Couldn't this be fixed more simply by dropping the losing weak definitions?


Maybe, maybe not. A few thoughts:

- Right now we don't drop non-prevailing LinkOnceAny or WeakAny to available_externally in thinLTOResolveWeakForLinkerGUID, but I suppose we could since although they are not guaranteed to be identical (as in the ODR case), we know which is prevailing and it can't be inlined anyway (isInterposableLinkage).
- We can't always drop to available_externally, e.g. in the case where this is an alias or aliasee.
- The original case that motivated my looking into this was more complicated, and in fact we did drop the non-prevailing copies. In that case we had 2 linkonce_odr copies, and both were in comdats. We dropped the non-prevailing copy to available_externally, but it was first in the static library so currently linked first in the native link. The problem was that the comdat group in the module with the non-prevailing copy was still non-empty, it looked like:

COMDAT group section [  238] `.group' [_ZN4foo_E] contains 2 sections:

  [Index]    Name
  [  239]   .ctors
  [  240]   .rela.ctors

With the wrong object ordering on the final link, this partial comdat group was selected and the one containing the intended prevailing copies was completely removed. (I don't have the final IR handy anymore, but the symbol was originally in the llvm.global_ctors list so presumably that's why we still end up with a comdat group like that?)

Perhaps there is a way to fix my original issue by removing the comdat and all of its constituents when we mark a symbol as available_externally in the backend, but I haven't thought through this too much since discovering the weak symbol issue in the test case here that is also related to the final native link order issue. Fixing that issue by dropping the non-prevailing weak symbol requires also fixing the comdat issue, as well as addressing the limitation we have on dropping non-prevailing copies that are involved with aliases (requires duplication of the aliased definition I believe).

In general, it seems like we're asking for trouble by performing the final native link in a different order than the linker decided on during the thin link. That being said, I don't love this solution, but I didn't think of a better way to determine that ordering.


https://reviews.llvm.org/D28523