Fix logic for which symbols to keep with comdats

[• rafael]

Summary

If a comdat is dropped, all symbols in it are dropped.
If a comdat is kept, the symbols survive to pass regular symbol resolution.
With this patch we do that for all global symbols.

The added test is a copy of test/tools/gold/X86/comdat.ll that we now pass.

[• rafael]

comment for phab to send email to the list.

[tejohnson]

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?

lib/Linker/LinkModules.cpp

425–426

We now only add comdat members to ValuesToLink on a case by case basis below.

567–568

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?

test/Linker/Inputs/comdat16.ll

6

Should test check that?

[• rafael]

Update test.

lib/Linker/LinkModules.cpp

bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
 
   if (GV.isDeclaration())
     return false;
 
   if (const Comdat *SC = GV.getComdat()) {
     bool LinkFromSrc;
     Comdat::SelectionKind SK;
     std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
-    if (LinkFromSrc)
-      ValuesToLink.insert(&GV);
+    if (!LinkFromSrc)
       return false;
   }
 
   bool LinkFromSrc = true;
   if (DGV && shouldLinkFromSource(LinkFromSrc, *DGV, GV))
     return true;
   if (LinkFromSrc)
     ValuesToLink.insert(&GV);
   return false;
if (linkIfNeeded(GA))
       return true;
 
   for (unsigned I = 0; I < ValuesToLink.size(); ++I) {
     GlobalValue *GV = ValuesToLink[I];
     const Comdat *SC = GV->getComdat();
     if (!SC)
       continue;
     for (GlobalValue *GV2 : ComdatMembers[SC])
+      if (GV2->hasInternalLinkage())
         ValuesToLink.insert(GV2);
   }
 
   if (shouldInternalizeLinkedSymbols()) {
     for (GlobalValue *GV : ValuesToLink)
       Internalize.insert(GV->getName());
   }
 
   if (Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),

test/Linker/Inputs/comdat16.ll

+$c2 = comdat any
+$c1 = comdat any
+
+; This is only present in this file. The linker will keep $c1 from the first
+; file and this will be undefined.
+@will_be_undefined = global i32 1, comdat($c1)
+@use = global i32* @will_be_undefined
+
+@v1 = weak_odr global i32 41, comdat($c2)
+define weak_odr protected i32 @f1(i8* %this) comdat($c2) {
+bb20:
+  store i8* %this, i8** null
+  br label %bb21
+bb21:
+  ret i32 41
+}
+
+@r21 = global i32* @v1
+@r22 = global i32(i8*)* @f1
+
+@a21 = alias i32, i32* @v1
+@a22 = alias i16, bitcast (i32* @v1 to i16*)
+
+@a23 = alias i32(i8*), i32(i8*)* @f1
+@a24 = alias i16, bitcast (i32(i8*)* @f1 to i16*)
+@a25 = alias i16, i16* @a24

test/Linker/comdat16.ll

+; RUN: llvm-link -S -o - %s %p/Inputs/comdat16.ll | FileCheck %s
+
+$c1 = comdat any
+
+@v1 = weak_odr global i32 42, comdat($c1)
+define weak_odr i32 @f1(i8*) comdat($c1) {
+bb10:
+  br label %bb11
+bb11:
+  ret i32 42
+}
+
+@r11 = global i32* @v1
+@r12 = global i32 (i8*)* @f1
+
+@a11 = alias i32, i32* @v1
+@a12 = alias i16, bitcast (i32* @v1 to i16*)
+
+@a13 = alias i32 (i8*), i32 (i8*)* @f1
+@a14 = alias i16, bitcast (i32 (i8*)* @f1 to i16*)
+@a15 = alias i16, i16* @a14
+
+; CHECK: $c1 = comdat any
+; CHECK: $c2 = comdat any
+
+; CHECK-DAG: @will_be_undefined = external global i32
+
+; CHECK-DAG: @v1 = weak_odr global i32 42, comdat($c1)
+
+; CHECK-DAG: @r11 = global i32* @v1{{$}}
+; CHECK-DAG: @r12 = global i32 (i8*)* @f1{{$}}
+
+; CHECK-DAG: @r21 = global i32* @v1{{$}}
+; CHECK-DAG: @r22 = global i32 (i8*)* @f1{{$}}
+
+; CHECK-DAG: @v1.1 = internal global i32 41, comdat($c2)
+
+; CHECK-DAG: @a11 = alias i32, i32* @v1{{$}}
+; CHECK-DAG: @a12 = alias i16, bitcast (i32* @v1 to i16*)
+
+; CHECK-DAG: @a13 = alias i32 (i8*), i32 (i8*)* @f1{{$}}
+; CHECK-DAG: @a14 = alias i16, bitcast (i32 (i8*)* @f1 to i16*)
+
+; CHECK-DAG: @a21 = alias i32, i32* @v1.1{{$}}
+; CHECK-DAG: @a22 = alias i16, bitcast (i32* @v1.1 to i16*)
+
+; CHECK-DAG: @a23 = alias i32 (i8*), i32 (i8*)* @f1.2{{$}}
+; CHECK-DAG: @a24 = alias i16, bitcast (i32 (i8*)* @f1.2 to i16*)
+
+; CHECK:      define weak_odr protected i32 @f1(i8*) comdat($c1) {
+; CHECK-NEXT: bb10:
+; CHECK-NEXT:   br label %bb11{{$}}
+; CHECK:      bb11:
+; CHECK-NEXT:   ret i32 42
+; CHECK-NEXT: }
+
+; CHECK:      define internal i32 @f1.2(i8* %this) comdat($c2) {
+; CHECK-NEXT: bb20:
+; CHECK-NEXT:   store i8* %this, i8** null
+; CHECK-NEXT:   br label %bb21
+; CHECK:      bb21:
+; CHECK-NEXT:   ret i32 41
+; CHECK-NEXT: }

test/Linker/constructor-comdat.ll

 ; RUN: llvm-link %s %p/Inputs/constructor-comdat.ll -S -o - 2>&1 | FileCheck %s
-; RUN: llvm-link %p/Inputs/constructor-comdat.ll %s -S -o - 2>&1 | FileCheck %s
+; RUN: llvm-link %p/Inputs/constructor-comdat.ll %s -S -o - 2>&1 | FileCheck --check-prefix=NOCOMDAT %s
 
 $_ZN3fooIiEC5Ev = comdat any
 ; CHECK: $_ZN3fooIiEC5Ev = comdat any
+; NOCOMDAT-NOT: comdat
 
 @_ZN3fooIiEC1Ev = weak_odr alias void (), void ()* @_ZN3fooIiEC2Ev
 ; CHECK: @_ZN3fooIiEC1Ev = weak_odr alias void (), void ()* @_ZN3fooIiEC2Ev
+; NOCOMDAT-DAG: define weak_odr void @_ZN3fooIiEC1Ev() {
 
 ; CHECK: define weak_odr void @_ZN3fooIiEC2Ev() comdat($_ZN3fooIiEC5Ev) {
+; NOCOMDAT-DAG: define weak_odr void @_ZN3fooIiEC2Ev() {
 define weak_odr void @_ZN3fooIiEC2Ev() comdat($_ZN3fooIiEC5Ev) {
   ret void
 }