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

1992

indentation seems off

[dschuff]

• fix indentation

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.

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::GlobalValue &GV);
 
   CGCXXABI &getCXXABI() const { return *ABI; }
   llvm::LLVMContext &getLLVMContext() { return VMContext; }
 
   bool shouldUseTBAA() const { return TBAA != nullptr; }
 
   const TargetCodeGenInfo &getTargetCodeGenInfo(); 
   

lib/CodeGen/CodeGenModule.cpp

 
 void CodeGenModule::maybeSetTrivialComdat(const Decl &D,
                                           llvm::GlobalObject &GO) {
   if (!shouldBeInCOMDAT(*this, D))
     return;
   GO.setComdat(TheModule.getOrInsertComdat(GO.getName()));
 }
 
+void CodeGenModule::maybeSetTrivialComdat(llvm::GlobalValue &GV) {
+  if (!supportsCOMDAT())
+    return;
+  if (GV.isWeakForLinker() && !GV.hasAvailableExternallyLinkage()) {
+    if (auto *GO = dyn_cast<llvm::GlobalObject>(&GV))
+      GO->setComdat(getModule().getOrInsertComdat(GV.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;

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{{.*}} }