Do not emit thunks with available_externally linkage in comdats

[dschuff]

Summary

Functions with available_externally linkage will not be emitted to object
files (they will just be undefined symbols), so it does not make sense to
put them in comdats.

[rnk]

This is correct, but we have lots of instances that should use the same logic:
$ git grep supportsCOMDAT.*isWeakForLinker
lib/CodeGen/CGDecl.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/CGVTT.cpp: if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
lib/CodeGen/CGVTables.cpp: if (CGM.supportsCOMDAT() && Fn->isWeakForLinker())
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker() &&
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: } else if (CGM.supportsCOMDAT() && guard->isWeakForLinker()) {
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && GV->isWeakForLinker())

How about we add a maybeSetTrivialComdat overload that takes a GlobalObject or something? I'm not asking you to do the migration if you don't want to, but just to add the central place that we should be calling to handle the case where we don't have an easily available Decl with GVA linkage.

[dschuff]

Yeah that was actually going to be my followup question, how many other places need this. My concern about just grepping and changing was just the issue I encountered here, where the linkage type was actually not finally set at the point where the comdat was added (hence moving it from generateThunk to emitThunk).

Also it looks like many of those cases can actually use the existing maybeAddTrivialComdat because they have Decls available. Anyway here's another patch with a maybeSetTrivialComdat overload.
Do you have an opinion about whether it should take a GlobalValue or just a GlobalObject? I'm a little bit unclear about linkage and comdats for aliases.

[dschuff]

• use a maybeSetTrivialComdat

[dschuff]

Actually the other places that needed fixing were all quite straightforward,
so just fix them too. Also make maybeSetTrivialComdat take a GO instead of a GV.

We could maybe even just replace the maybeSetTrivialComdat that takes the Decl
too, but that's probably another CL.

[rnk]

Thanks! lgtm

lib/CodeGen/CodeGenModule.cpp

1989 ↗

(On Diff #25342)

indentation seems off

[dschuff]

• fix indentation

cfe/trunk/lib/CodeGen/CGDecl.cpp

llvm::GlobalVariable *GV =
     new llvm::GlobalVariable(getModule(), LTy,
                              Ty.isConstant(getContext()), Linkage,
                              Init, Name, nullptr,
                              llvm::GlobalVariable::NotThreadLocal,
                              AddrSpace);
   GV->setAlignment(getContext().getDeclAlign(&D).getQuantity());
   setGlobalVisibility(GV, &D);
 
-  if (supportsCOMDAT() && GV->isWeakForLinker())
-    GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
+  maybeSetTrivialComdat(*GV);
 
   if (D.getTLSKind())
     setTLSMode(GV, D);
 
   if (D.isExternallyVisible()) {
     if (D.hasAttr<DLLImportAttr>())
       GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
     else if (D.hasAttr<DLLExportAttr>())

cfe/trunk/lib/CodeGen/CGVTT.cpp

CodeGenVTables::EmitVTTDefinition(llvm::GlobalVariable *VTT,
 
   llvm::Constant *Init = llvm::ConstantArray::get(ArrayType, VTTComponents);
 
   VTT->setInitializer(Init);
 
   // Set the correct linkage.
   VTT->setLinkage(Linkage);
 
-  if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
-    VTT->setComdat(CGM.getModule().getOrInsertComdat(VTT->getName()));
+  CGM.maybeSetTrivialComdat(*VTT);
 
   // Set the right visibility.
   CGM.setGlobalVisibility(VTT, RD);
 }
 
 llvm::GlobalVariable *CodeGenVTables::GetAddrOfVTT(const CXXRecordDecl *RD) {
   assert(RD->getNumVBases() && "Only classes with virtual bases need a VTT");
 
for (llvm::DenseMap<BaseSubobject, uint64_t>::const_iterator I =
     SecondaryVirtualPointerIndices.insert(std::make_pair(Pair, I->second));
   }
 
   I = SecondaryVirtualPointerIndices.find(std::make_pair(RD, Base));
   assert(I != SecondaryVirtualPointerIndices.end() && "Did not find index!");
   
   return I->second;
 }
-

cfe/trunk/lib/CodeGen/CGVTables.cpp

void CodeGenFunction::GenerateThunk(llvm::Function *Fn,
   llvm::Value *Callee = CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
 
   // Make the call and return the result.
   EmitCallAndReturnForThunk(Callee, &Thunk);
 
   // Set the right linkage.
   CGM.setFunctionLinkage(GD, Fn);
 
-  if (CGM.supportsCOMDAT() && Fn->isWeakForLinker())
-    Fn->setComdat(CGM.getModule().getOrInsertComdat(Fn->getName()));
-
   // Set the right visibility.
   const CXXMethodDecl *MD = cast<CXXMethodDecl>(GD.getDecl());
   setThunkVisibility(CGM, MD, Thunk, Fn);
 }
 
 void CodeGenVTables::emitThunk(GlobalDecl GD, const ThunkInfo &Thunk,
                                bool ForVTable) {
   const CGFunctionInfo &FnInfo = CGM.getTypes().arrangeGlobalDeclaration(GD);
if (!UseAvailableExternallyLinkage) {
                                       !Thunk.Return.isEmpty());
     }
   } else {
     // Normal thunk body generation.
     CodeGenFunction(CGM).GenerateThunk(ThunkFn, FnInfo, GD, Thunk);
     CGM.getCXXABI().setThunkLinkage(ThunkFn, ForVTable, GD,
                                     !Thunk.Return.isEmpty());
   }
+  CGM.maybeSetTrivialComdat(*ThunkFn);
 }
 
 void CodeGenVTables::maybeEmitThunkForVTable(GlobalDecl GD,
                                              const ThunkInfo &Thunk) {
   // If the ABI has key functions, only the TU with the key function should emit
   // the thunk. However, we can allow inlining of thunks if we emit them with
   // available_externally linkage together with vtables when optimizations are
   // enabled.

cfe/trunk/lib/CodeGen/CodeGenModule.h

public:
   const CodeGenOptions &getCodeGenOpts() const { return CodeGenOpts; }
   llvm::Module &getModule() const { return TheModule; }
   DiagnosticsEngine &getDiags() const { return Diags; }
   const llvm::DataLayout &getDataLayout() const { return TheDataLayout; }
   const TargetInfo &getTarget() const { return Target; }
   const llvm::Triple &getTriple() const;
   bool supportsCOMDAT() const;
   void maybeSetTrivialComdat(const Decl &D, llvm::GlobalObject &GO);
+  void maybeSetTrivialComdat(llvm::GlobalObject &GO);
 
   CGCXXABI &getCXXABI() const { return *ABI; }
   llvm::LLVMContext &getLLVMContext() { return VMContext; }
 
   bool shouldUseTBAA() const { return TBAA != nullptr; }
 
   const TargetCodeGenInfo &getTargetCodeGenInfo(); 
   

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

if (llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name))
     return GV;
 
   llvm::Constant *Init = EmitUuidofInitializer(Uuid);
   assert(Init && "failed to initialize as constant");
 
   auto *GV = new llvm::GlobalVariable(
       getModule(), Init->getType(),
       /*isConstant=*/true, llvm::GlobalValue::LinkOnceODRLinkage, Init, Name);
-  if (supportsCOMDAT())
-    GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
+  maybeSetTrivialComdat(*GV);
   return GV;
 }
 
 llvm::Constant *CodeGenModule::GetWeakRefReference(const ValueDecl *VD) {
   const AliasAttr *AA = VD->getAttr<AliasAttr>();
   assert(AA && "No alias?");
 
   llvm::Type *DeclTy = getTypes().ConvertTypeForMem(VD->getType());
if (!OldGV->use_empty()) {
       llvm::Constant *NewPtrForOldDecl =
       llvm::ConstantExpr::getBitCast(GV, OldGV->getType());
       OldGV->replaceAllUsesWith(NewPtrForOldDecl);
     }
     
     OldGV->eraseFromParent();
   }
 
-  if (supportsCOMDAT() && GV->isWeakForLinker() &&
-      !GV->hasAvailableExternallyLinkage())
-    GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
+  maybeSetTrivialComdat(*GV);
 
   return GV;
 }
 
 /// GetAddrOfGlobalVar - Return the llvm::Constant for the address of the
 /// given global variable.  If Ty is non-null and if the global doesn't exist,
 /// then it will be created with the specified type instead of whatever the
 /// normal requested type would be.

 
 void CodeGenModule::maybeSetTrivialComdat(const Decl &D,
                                           llvm::GlobalObject &GO) {
   if (!shouldBeInCOMDAT(*this, D))
     return;
   GO.setComdat(TheModule.getOrInsertComdat(GO.getName()));
 }
 
+void CodeGenModule::maybeSetTrivialComdat(llvm::GlobalObject &GO) {
+  if (!supportsCOMDAT())
+    return;
+  if (GO.isWeakForLinker() && !GO.hasAvailableExternallyLinkage()) {
+    GO.setComdat(getModule().getOrInsertComdat(GO.getName()));
+  }
+}
+
 void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) {
   llvm::Constant *Init = nullptr;
   QualType ASTTy = D->getType();
   CXXRecordDecl *RD = ASTTy->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
   bool NeedsGlobalCtor = false;
   bool NeedsGlobalDtor = RD && !RD->hasTrivialDestructor();
 
   const VarDecl *InitDecl;
GenerateStringLiteral(llvm::Constant *C, llvm::GlobalValue::LinkageTypes LT,
 
   llvm::Module &M = CGM.getModule();
   // Create a global variable for this string
   auto *GV = new llvm::GlobalVariable(
       M, C->getType(), !CGM.getLangOpts().WritableStrings, LT, C, GlobalName,
       nullptr, llvm::GlobalVariable::NotThreadLocal, AddrSpace);
   GV->setAlignment(Alignment);
   GV->setUnnamedAddr(true);
-  if (GV->isWeakForLinker()) {
-    assert(CGM.supportsCOMDAT() && "Only COFF uses weak string literals");
-    GV->setComdat(M.getOrInsertComdat(GV->getName()));
-  }
+  CGM.maybeSetTrivialComdat(*GV);
 
   return GV;
 }
 
 /// GetAddrOfConstantStringFromLiteral - Return a pointer to a
 /// constant array for the given string literal.
 llvm::GlobalVariable *
 CodeGenModule::GetAddrOfConstantStringFromLiteral(const StringLiteral *S,
unsigned AddrSpace = GetGlobalVarAddressSpace(
       VD, getContext().getTargetAddressSpace(MaterializedType));
   auto *GV = new llvm::GlobalVariable(
       getModule(), Type, Constant, Linkage, InitialValue, Name.c_str(),
       /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal,
       AddrSpace);
   setGlobalVisibility(GV, VD);
   GV->setAlignment(
       getContext().getTypeAlignInChars(MaterializedType).getQuantity());
-  if (supportsCOMDAT() && GV->isWeakForLinker())
-    GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
+  maybeSetTrivialComdat(*GV);
   if (VD->getTLSKind())
     setTLSMode(GV, *VD);
   Slot = GV;
   return GV;
 }
 
 /// EmitObjCPropertyImplementations - Emit information for synthesized
 /// properties for an implementation.

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
   llvm::Constant *Init = CGVT.CreateVTableInitializer(
       RD, VTLayout.vtable_component_begin(), VTLayout.getNumVTableComponents(),
       VTLayout.vtable_thunk_begin(), VTLayout.getNumVTableThunks(), RTTI);
   VTable->setInitializer(Init);
 
   // Set the correct linkage.
   VTable->setLinkage(Linkage);
 
-  if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
-    VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
+  CGM.maybeSetTrivialComdat(*VTable);
 
   // Set the right visibility.
   CGM.setGlobalVisibility(VTable, RD);
 
   // Use pointer alignment for the vtable. Otherwise we would align them based
   // on the size of the initializer which doesn't make sense as only single
   // values are read.
   unsigned PAlign = CGM.getTarget().getPointerAlign(0);
if (!guard) {
     guard->setThreadLocalMode(var->getThreadLocalMode());
 
     // The ABI says: It is suggested that it be emitted in the same COMDAT group
     // as the associated data object
     llvm::Comdat *C = var->getComdat();
     if (!D.isLocalVarDecl() && C) {
       guard->setComdat(C);
       CGF.CurFn->setComdat(C);
-    } else if (CGM.supportsCOMDAT() && guard->isWeakForLinker()) {
-      guard->setComdat(CGM.getModule().getOrInsertComdat(guard->getName()));
+    } else {
+      CGM.maybeSetTrivialComdat(*guard);
     }
 
     CGM.setStaticLocalDeclGuardAddress(&D, guard);
   }
 
   // Test whether the variable has completed initialization.
   //
   // Itanium C++ ABI 3.3.2:
#include "clang/AST/TypeNodes.def"
 
   llvm::Constant *Init = llvm::ConstantStruct::getAnon(Fields);
 
   llvm::Module &M = CGM.getModule();
   llvm::GlobalVariable *GV =
       new llvm::GlobalVariable(M, Init->getType(),
                                /*Constant=*/true, Linkage, Init, Name);
 
-  if (CGM.supportsCOMDAT() && GV->isWeakForLinker())
-    GV->setComdat(M.getOrInsertComdat(GV->getName()));
+  CGM.maybeSetTrivialComdat(*GV);
 
   // If there's already an old global variable, replace it with the new one.
   if (OldGV) {
     GV->takeName(OldGV);
     llvm::Constant *NewPtr =
       llvm::ConstantExpr::getBitCast(GV, OldGV->getType());
     OldGV->replaceAllUsesWith(NewPtr);
     OldGV->eraseFromParent();
if (fn && fn->empty()) {
     // forbidding it completely.  The difference between that and
     // 'noinline' is negligible.
     fn->addFnAttr(llvm::Attribute::NoInline);
 
     // Allow this function to be shared across translation units, but
     // we don't want it to turn into an exported symbol.
     fn->setLinkage(llvm::Function::LinkOnceODRLinkage);
     fn->setVisibility(llvm::Function::HiddenVisibility);
-    if (CGM.supportsCOMDAT())
-      fn->setComdat(CGM.getModule().getOrInsertComdat(fn->getName()));
+    CGM.maybeSetTrivialComdat(*fn);
 
     // Set up the function.
     llvm::BasicBlock *entry =
       llvm::BasicBlock::Create(CGM.getLLVMContext(), "", fn);
     CGBuilderTy builder(entry);
 
     // Pull the exception pointer out of the parameter list.
     llvm::Value *exn = &*fn->arg_begin();

cfe/trunk/test/CodeGenCXX/thunks.cpp

 // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - -O1 -disable-llvm-optzns | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - -O1 -disable-llvm-optzns | FileCheck %s --check-prefix=CHECK --check-prefix=CHECKOPT
 
 namespace Test1 {
 
 // Check that we emit a non-virtual thunk for C::f.
 
 struct A {
   virtual void f();
 };
namespace Test15 {
   void C::c() {}
 
   // C::c
   // CHECK: declare void @_ZN6Test151C1fEiz
   // non-virtual thunk to C::f
   // CHECK: declare void @_ZThn8_N6Test151C1fEiz
 }
 
+namespace Test16 {
+
+// Check that the thunk for 'B::f' has available_externally linkage
+// and is not in a comdat.
+
+template <class C>
+struct A {
+  virtual void f();
+};
+
+template <class D>
+struct B : virtual A<D> {
+  virtual void f() { }
+};
+
+extern template struct B<int>;
+
+void f(B<int> b) {
+  b.f();
+}
+}
+
 /**** The following has to go at the end of the file ****/
 
 // This is from Test5:
 // CHECK-LABEL: define internal void @_ZThn8_N6Test4B12_GLOBAL__N_11C1fEv(
 // CHECK-LABEL: define linkonce_odr void @_ZTv0_n24_N5Test51B1fEv
 
 // This is from Test10:
 // CHECK-LABEL: define linkonce_odr void @_ZN6Test101C3fooEv
 // CHECK-LABEL: define linkonce_odr void @_ZThn8_N6Test101C3fooEv
 
+// CHECKOPT-LABEL: define available_externally void @_ZTv0_n24_N6Test161BIiE1fEv
+// CHECKOPT-NOT: comdat
+
 // CHECK: attributes [[NUW]] = { nounwind uwtable{{.*}} }