[LTO] Remove values from non-prevailing comdats

Summary:
When linking a regular LTO module, if it has any non-prevailing values
(dropped to available_externally) in comdats, we need to do more than
just remove those values from their comdat. We also remove all values
from that comdat, so as to avoid leaving an incomplete comdat.

This is necessary in case we are compiling in mixed regular and ThinLTO
mode, since the resulting regularLTO native object is always linked into
the final binary first. We need to prevent the linker from selecting an
incomplete comdat that was not the prevailing copy.

Fixes PR32980.

Reviewers: pcc, rafael

Subscribers: mehdi_amini, david2050, llvm-commits, inglorion

Differential Revision: https://reviews.llvm.org/D34803

llvm-svn: 306826

lib/LTO/LTO.cpp

@@ -472,6 +472,36 @@ Error LTO::addModule(InputFile &Input, unsigned ModI,
   return Error::success();
 }
 
+// Checks whether the given global value is in a non-prevailing comdat
+// (comdat containing values the linker indicated were not prevailing,
+// which we then dropped to available_externally), and if so, removes
+// it from the comdat. This is called for all global values to ensure the
+// comdat is empty rather than leaving an incomplete comdat. It is needed for
+// regular LTO modules, in case we are in a mixed-LTO mode (both regular
+// and thin LTO modules) compilation. Since the regular LTO module will be
+// linked first in the final native link, we want to make sure the linker
+// doesn't select any of these incomplete comdats that would be left
+// in the regular LTO module without this cleanup.
+static void
+handleNonPrevailingComdat(GlobalValue &GV,
+                          std::set<const Comdat *> &NonPrevailingComdats) {
+  Comdat *C = GV.getComdat();
+  if (!C)
+    return;
+
+  if (!NonPrevailingComdats.count(C))
+    return;
+
+  // Additionally need to drop externally visible global values from the comdat
+  // to available_externally, so that there aren't multiply defined linker
+  // errors.
+  if (!GV.hasLocalLinkage())
+    GV.setLinkage(GlobalValue::AvailableExternallyLinkage);
+
+  if (auto GO = dyn_cast<GlobalObject>(&GV))
+    GO->setComdat(nullptr);
+}
+
 // Add a regular LTO object to the link.
 // The resulting module needs to be linked into the combined LTO module with
 // linkRegularLTO.
@@ -523,6 +553,7 @@ LTO::addRegularLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
   };
   Skip();
 
+  std::set<const Comdat *> NonPrevailingComdats;
   for (const InputFile::Symbol &Sym : Syms) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
@@ -557,6 +588,8 @@ LTO::addRegularLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
         // module (in linkRegularLTO), based on whether it is undefined.
         Mod.Keep.push_back(GV);
         GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
+        if (GV->hasComdat())
+          NonPrevailingComdats.insert(GV->getComdat());
         cast<GlobalObject>(GV)->setComdat(nullptr);
       }
     }
@@ -574,6 +607,9 @@ LTO::addRegularLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
+  if (!M.getComdatSymbolTable().empty())
+    for (GlobalValue &GV : M.global_values())
+      handleNonPrevailingComdat(GV, NonPrevailingComdats);
   assert(MsymI == MsymE);
   return std::move(Mod);
 }

test/LTO/Resolution/X86/Inputs/comdat-mixed-lto.ll

@@ -0,0 +1,23 @@
+; ModuleID = 'comdat-mixed-lto1.o'
+source_filename = "comdat-mixed-lto1.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%"class.Test::ptr" = type { i32 }
+
+$C = comdat any
+
+@C = linkonce_odr global %"class.Test::ptr" zeroinitializer, comdat, align 4
+@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @__cxx_global_var_init, i8* bitcast (%"class.Test::ptr"* @C to i8*) }]
+
+define void @testglobfunc() #1 section ".text.startup" comdat($C) {
+entry:
+  ret void
+}
+
+; Function Attrs: noinline uwtable
+define internal void @__cxx_global_var_init() #1 section ".text.startup" comdat($C) {
+entry:
+  store i32 0, i32* getelementptr inbounds (%"class.Test::ptr", %"class.Test::ptr"* @C, i32 0, i32 0), align 4
+  ret void
+}

test/LTO/Resolution/X86/comdat-mixed-lto.ll

@@ -0,0 +1,42 @@
+; Test of comdat handling with mixed thinlto and regular lto compilation.
+
+; This module is compiled with ThinLTO
+; RUN: opt -module-summary -o %t1.o %s
+; Input module compiled for regular LTO
+; RUN: opt -o %t2.o %p/Inputs/comdat-mixed-lto.ll
+
+; The copy of C from this module is prevailing. The copy of C from the
+; regular LTO module is not prevailing, and will be dropped to
+; available_externally.
+; RUN: llvm-lto2 run -r=%t1.o,C,pl -r=%t2.o,C,l -r=%t2.o,testglobfunc,lxp -r=%t1.o,testglobfunc,lx -o %t3 %t1.o %t2.o -save-temps
+
+; The Input module (regular LTO) is %t3.0. Check to make sure that we removed
+; __cxx_global_var_init and testglobfunc from comdat. Also check to ensure
+; that testglobfunc was dropped to available_externally. Otherwise we would
+; have linker multiply defined errors as it is no longer in a comdat and
+; would clash with the copy from this module.
+; RUN: llvm-dis %t3.0.0.preopt.bc -o - | FileCheck %s
+; CHECK: define internal void @__cxx_global_var_init() section ".text.startup" {
+; CHECK: define available_externally void @testglobfunc() section ".text.startup" {
+
+; ModuleID = 'comdat-mixed-lto.o'
+source_filename = "comdat-mixed-lto.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%"class.Test::ptr" = type { i32 }
+
+$C = comdat any
+
+@C = linkonce_odr global %"class.Test::ptr" zeroinitializer, comdat, align 4
+@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @__cxx_global_var_init, i8* bitcast (%"class.Test::ptr"* @C to i8*) }]
+define void @testglobfunc() #1 section ".text.startup" comdat($C) {
+entry:
+  ret void
+}
+
+; Function Attrs: noinline uwtable
+define internal void @__cxx_global_var_init() #1 section ".text.startup" comdat($C) {
+entry:
+  ret void
+}