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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Still think we have a potential issue, see comments embedded in the odr_resolution test case.


================
Comment at: include/llvm/Linker/Linker.h:33
@@ +32,3 @@
+    InternalizeLinkedSymbols = (1 << 2),
+    /// Don't link the definition referenced linkonce.
+    DontLazyAddLinkone = (1<<3)
----------------
Suggest as clearer:

/// Don't force link referenced linkonce definitions.

(i.e. "linkonce definitions" instead of "definition ... linkonce")

================
Comment at: include/llvm/Linker/Linker.h:34
@@ -33,1 +33,3 @@
+    /// Don't link the definition referenced linkonce.
+    DontLazyAddLinkone = (1<<3)
   };
----------------
s/DontLazyAddLinkone/DontLazyAddLinkonce/ (s/one/once)

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:162
@@ -152,2 +161,3 @@
       return GlobalValue::WeakODRLinkage;
-    else
+    else if(!GlobalInvolvedWithAlias.count(&GV))
+      // Alias cannot point to an AvailableExternallyLinkage
----------------
Missing space after 'if'.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:163
@@ -154,1 +162,3 @@
+    else if(!GlobalInvolvedWithAlias.count(&GV))
+      // Alias cannot point to an AvailableExternallyLinkage
       return GlobalValue::AvailableExternallyLinkage;
----------------
It took me some time to convince myself that this was going to be correct in all cases. Specifically, I was concerned about the case where an imported function referenced an aliasee that remained linkonce (wasn't first def for linker), and we ended up trying to create a linkonce declaration (illegal) due to the change in addLazyFor. Then I saw the code in the IRMover that turns such declarations into ExternalWeak (as you imply in your question to Rafael in the test case, it doesn't need to be extern_weak, but the important point is that it isn't left linkonce_odr, and we will have a copy emitted in one of the modules for the linker to resolve it with).

I think it would be helpful to add a comment here about how this works. I.e. something like "If aliasee that remains linkonce is referenced by an imported function, the IRMover will convert the declaration to ExternalWeak, which will be satisfied at link time with the copy that was resolved to WeakODR."


================
Comment at: lib/Linker/LinkModules.cpp:435
@@ +434,3 @@
+    // The client takes care of linkonce availability.
+    return;
+
----------------
> 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)!

It is unsound once the thin link step starts making optimizations based on the importing decisions, which is a good direction to go in. But first we have to get all of the ThinLTO clients to handle all linkonce cases correctly. This patch is a good start, and similar handling can be added to the gold-plugin and distributed backend cases. Eventually if the resolved linkage is updated in the index in all cases, we can add some assertion checking to ensure that we don't end up importing a reference to something that remains linkonce but doesn't have its definition in the GlobalsToLink set.

================
Comment at: test/ThinLTO/X86/odr_resolution.ll:13
@@ -12,3 +12,3 @@
 
 ; Alias are not optimized
 ; MOD1: @linkoncealias = linkonce_odr alias void (), void ()* @linkonceodrfuncwithalias
----------------
Can you remind me how we prevent the alias from being optimized this way? We have AliasSummary in DefinedGlobals, but only the aliasee are added to the GlobalInvolvedWithAlias set. I don't see where it is preventing the ODR resolution for the alias itself. If we did try to do the alias resolution, we would end up with an available_externally alias in MOD2, which is illegal.

If we do in fact prevent the ODR resolution in this case, then I think there is a different issue. If we have a reference to the alias in an imported routine, with the change to addLazyFor() we would no longer import this linkonce alias as a definition, and the IRMover will convert the imported declaration to ExternalWeak. But with all the copies of the defintion remaining linkonce, we aren't guaranteed to emit a copy anywhere, they could all be eliminated after inlining in their original modules, and we have an undef at link time.


http://reviews.llvm.org/D19096