[PATCH] D18390: Fix logic for which symbols to keep with comdats
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 23 13:06:06 PDT 2016
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)
- 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])
+ if (GV2->hasInternalLinkage())
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?
More information about the llvm-commits