[PATCH] D18390: Fix logic for which symbols to keep with comdats

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

I'm not sure how these changes are fixing the issue described - we used to always add to ValuesToLink any GV in a chosen comdat, but here we only sometimes do. Can't that result in incomplete comdat groups?

I tried the new test case with HEAD and see that it doesn't have a $c1 comdat at all, but not sure why or how that is fixed by the patch. I'm obviously missing something, can you give me a better idea of how this is working?


================
Comment at: lib/Linker/LinkModules.cpp:425
@@ -424,4 +424,3 @@
     std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
-    if (LinkFromSrc)
-      ValuesToLink.insert(&GV);
-    return false;
+    if (!LinkFromSrc)
+      return false;
----------------
We now only add comdat members to ValuesToLink on a case by case basis below.

================
Comment at: lib/Linker/LinkModules.cpp:567
@@ -567,2 +566,3 @@
     for (GlobalValue *GV2 : ComdatMembers[SC])
-      ValuesToLink.insert(GV2);
+      if (GV2->hasInternalLinkage())
+        ValuesToLink.insert(GV2);
----------------
Why only internal members? If we have a comdat member in ValuesToLink (added in linkIfNeeded), then presumably we had selected that copy of the comdat and wouldn't we want all members?

================
Comment at: test/Linker/Inputs/comdat16.ll:5
@@ +4,3 @@
+; This is only present in this file. The linker will keep $c1 from the first
+; file and this will be undefined.
+ at will_be_undefined = global i32 1, comdat($c1)
----------------
Should test check that?


http://reviews.llvm.org/D18390