[PATCH] D19096: ModuleLinker: do not always pull-in linkonce_odr when performing ThinLTO importing

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Linker/LinkModules.cpp:434
@@ +433,3 @@
+    // For ThinLTO we don't import more than what was required.
+    return;
+
----------------
tejohnson wrote:
> Noting down the concern I had expressed on IRC yesterday:
> 
> This is only legal if the LinkOnce is being eventually changed to something non-discardable, e.g. with the outstanding patches for libLTO (D18908) and gold (D18674, which applies when launching the backend threads through gold). However, even with those there are currently issues because neither patch handles linkonce that are involved with an alias. Any that will remain linkonce, because they aren't involved with an alias, or aren't covered by one of the two noted patches (e.g. using distributed back ends for ThinLTO), still need to be force imported or there will be failures (because the linkonce is discardable in its original exporting module).
> 
> My suggestion for fixing this is:
> 1) Cover aliases, either by allowing them to be resolved (via the proposed copy technique) or force import them when we collect the imports/exports lists from the index;
> and
> 2) Indicate to the module linker when it is safe to ignore linkonce here, either by noting the new linkage in the index (which needs to be passed in here somehow), or use a flag to tell the module linker that it is safe to ignore linkonce here when importing (only for libLTO for now, when #1 above is addressed).
> 
Yes this is only legal if the client code handles it correctly.
I'd like to remove totally any "magic" in general: it adds coupling and make the design too monolithic/complex.  Here the client is passing a list of globals to import, I don't expect the "module linker" to import anything else.

I think we talked a few weeks ago about that: ThinLTO importing should be using only the IRMover, not the Linker.
You were concerned about decision that would be taken in the backend that wouldn't match what the ThinLink decided: this is such a good case. The key point is that you have two different logic for the decision (summary based and non-summary based). This is quite unsafe (and unsound)!

I updated the patch to add a ModuleLinker flag that controls if referenced linkonce are imported or not, and I also improve the handling for aliases.


http://reviews.llvm.org/D19096