[CodeGen] Fix CodeGenModule::CreateGlobalInitOrDestructFunction

[ahatanak]

Summary

This patch makes the following changes to CodeGenModule::CreateGlobalInitOrDestructFunction:

• Use SetInternalFunctionAttributes instead of SetLLVMFunctionAttributes. Some of the attributes (e.g., stack protector strong) are added in SetLLVMFunctionAttributesForDefinition, which gets called from SetInternalFunctionAttributes.
• Pass the correct CGFunctionInfo to SetInternalFunctionAttributes instead of always passing what arrangeNullaryFunction returns. arrangeNullaryFunction returns the CGFunctionInfo for functions of type void(), but some of the functions are of different types. I haven't found a case where clang mis-compiles because of this bug, but nevertheless I think we should pass the correct type here.

[ahatanak]

Add a comment to send this patch to cfe-commits.

[echristo]

Can you give me some more background here? I'm not sure I see what's wrong and what's going wrong.

Thanks!

-eric

[ahatanak]

The patch isn't fixing any serious bugs, but is fixing what seems to me inconsistencies in the code. It makes the following changes:

1. Currently, SetLLVMFunctionAttributes is called to add function attributes to the internal function definitions, but SetLLVMFunctionAttributes doesn't call SetLLVMFunctionAttributesForDefinition which should be called to add function attributes to function definitions (but not to declarations). This patch replaces calls to SetLLVMFunctionAttributes with calls to SetInternalFunctionAttributes, which I think is the right thing to do since the functions created are all internal functions.

2. Currently, CodeGenModule::CreateGlobalInitOrDestructFunction always passes a CGFunctionInfo instance for void() functions (created by arrangeNullaryFunction) to SetLLVMFunctionAttributes. Strictly speaking, this is not correct for CodeGenFunction::generateDestroyHelper (near line 582) because the function type is void(*void.). That's why I made changes to have the callers of CreateGlobalInitOrDestructFunction pass a reference to CGFunctionInfo.

[echristo]

LGTM.

Thanks!

-eric

[loladiro]

Thanks for this. I was about to create the same patch (after seeing this broken on 3.7) only to see it had already been done!

cfe/trunk/lib/CodeGen/CGDeclCXX.cpp

llvm::Constant *CodeGenFunction::createAtExitStub(const VarDecl &VD,
                                                   llvm::Constant *addr) {
   // Get the destructor function type, void(*)(void).
   llvm::FunctionType *ty = llvm::FunctionType::get(CGM.VoidTy, false);
   SmallString<256> FnName;
   {
     llvm::raw_svector_ostream Out(FnName);
     CGM.getCXXABI().getMangleContext().mangleDynamicAtExitDestructor(&VD, Out);
   }
+
+  const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
   llvm::Function *fn = CGM.CreateGlobalInitOrDestructFunction(ty, FnName.str(),
+                                                              FI,
                                                               VD.getLocation());
 
   CodeGenFunction CGF(CGM);
 
-  CGF.StartFunction(&VD, CGM.getContext().VoidTy, fn,
-                    CGM.getTypes().arrangeNullaryFunction(), FunctionArgList());
+  CGF.StartFunction(&VD, CGM.getContext().VoidTy, fn, FI, FunctionArgList());
 
   llvm::CallInst *call = CGF.Builder.CreateCall(dtor, addr);
  
  // Make sure the call and the callee agree on calling convention.
   if (llvm::Function *dtorFn =
         dyn_cast<llvm::Function>(dtor->stripPointerCasts()))
     call->setCallingConv(dtorFn->getCallingConv());
 
if (CGM.getCodeGenOpts().ForbidGuardVariables)
     CGM.Error(D.getLocation(),
               "this initialization requires a guard variable, which "
               "the kernel does not support");
 
   CGM.getCXXABI().EmitGuardedInit(*this, D, DeclPtr, PerformInit);
 }
 
 llvm::Function *CodeGenModule::CreateGlobalInitOrDestructFunction(
-    llvm::FunctionType *FTy, const Twine &Name, SourceLocation Loc, bool TLS) {
+    llvm::FunctionType *FTy, const Twine &Name, const CGFunctionInfo &FI,
+    SourceLocation Loc, bool TLS) {
   llvm::Function *Fn =
     llvm::Function::Create(FTy, llvm::GlobalValue::InternalLinkage,
                            Name, &getModule());
   if (!getLangOpts().AppleKext && !TLS) {
     // Set the section if needed.
     if (const char *Section = getTarget().getStaticInitSectionSpecifier())
       Fn->setSection(Section);
   }
 
-  SetLLVMFunctionAttributes(nullptr, getTypes().arrangeNullaryFunction(), Fn);
+  SetInternalFunctionAttributes(nullptr, Fn, FI);
 
   Fn->setCallingConv(getRuntimeCC());
 
   if (!getLangOpts().Exceptions)
     Fn->setDoesNotThrow();
 
   if (!isInSanitizerBlacklist(Fn, Loc)) {
     if (getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address |
CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
   SmallString<256> FnName;
   {
     llvm::raw_svector_ostream Out(FnName);
     getCXXABI().getMangleContext().mangleDynamicInitializer(D, Out);
   }
 
   // Create a variable initialization function.
   llvm::Function *Fn =
-      CreateGlobalInitOrDestructFunction(FTy, FnName.str(), D->getLocation());
+      CreateGlobalInitOrDestructFunction(FTy, FnName.str(),
+                                         getTypes().arrangeNullaryFunction(),
+                                         D->getLocation());
 
   auto *ISA = D->getAttr<InitSegAttr>();
   CodeGenFunction(*this).GenerateCXXGlobalVarDeclInitFunc(Fn, D, Addr,
                                                           PerformInit);
 
   llvm::GlobalVariable *COMDATKey =
       supportsCOMDAT() && D->isExternallyVisible() ? Addr : nullptr;
 

 CodeGenModule::EmitCXXGlobalInitFunc() {
   while (!CXXGlobalInits.empty() && !CXXGlobalInits.back())
     CXXGlobalInits.pop_back();
 
   if (CXXGlobalInits.empty() && PrioritizedCXXGlobalInits.empty())
     return;
 
   llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
-
+  const CGFunctionInfo &FI = getTypes().arrangeNullaryFunction();
 
   // Create our global initialization function.
   if (!PrioritizedCXXGlobalInits.empty()) {
     SmallVector<llvm::Function *, 8> LocalCXXGlobalInits;
     llvm::array_pod_sort(PrioritizedCXXGlobalInits.begin(), 
                          PrioritizedCXXGlobalInits.end());
     // Iterate over "chunks" of ctors with same priority and emit each chunk
     // into separate function. Note - everything is sorted first by priority,
     // second - by lex order, so we emit ctor functions in proper order.
     for (SmallVectorImpl<GlobalInitData >::iterator
            I = PrioritizedCXXGlobalInits.begin(),
            E = PrioritizedCXXGlobalInits.end(); I != E; ) {
       SmallVectorImpl<GlobalInitData >::iterator
         PrioE = std::upper_bound(I + 1, E, *I, GlobalInitPriorityCmp());
 
       LocalCXXGlobalInits.clear();
       unsigned Priority = I->first.priority;
       // Compute the function suffix from priority. Prepend with zeroes to make
       // sure the function names are also ordered as priorities.
       std::string PrioritySuffix = llvm::utostr(Priority);
       // Priority is always <= 65535 (enforced by sema).
       PrioritySuffix = std::string(6-PrioritySuffix.size(), '0')+PrioritySuffix;
       llvm::Function *Fn = CreateGlobalInitOrDestructFunction(
-          FTy, "_GLOBAL__I_" + PrioritySuffix);
+          FTy, "_GLOBAL__I_" + PrioritySuffix, FI);
 
       for (; I < PrioE; ++I)
         LocalCXXGlobalInits.push_back(I->second);
 
       CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, LocalCXXGlobalInits);
       AddGlobalCtor(Fn, Priority);
     }
     PrioritizedCXXGlobalInits.clear();
CodeGenModule::EmitCXXGlobalInitFunc() {
   for (size_t i = 0; i < FileName.size(); ++i) {
     // Replace everything that's not [a-zA-Z0-9._] with a _. This set happens
     // to be the set of C preprocessing numbers.
     if (!isPreprocessingNumberBody(FileName[i]))
       FileName[i] = '_';
   }
 
   llvm::Function *Fn = CreateGlobalInitOrDestructFunction(
-      FTy, llvm::Twine("_GLOBAL__sub_I_", FileName));
+      FTy, llvm::Twine("_GLOBAL__sub_I_", FileName), FI);
 
   CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, CXXGlobalInits);
   AddGlobalCtor(Fn);
 
   CXXGlobalInits.clear();
 }
 
 void CodeGenModule::EmitCXXGlobalDtorFunc() {
   if (CXXGlobalDtors.empty())
     return;
 
   llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
 
   // Create our global destructor function.
-  llvm::Function *Fn = CreateGlobalInitOrDestructFunction(FTy, "_GLOBAL__D_a");
+  const CGFunctionInfo &FI = getTypes().arrangeNullaryFunction();
+  llvm::Function *Fn =
+      CreateGlobalInitOrDestructFunction(FTy, "_GLOBAL__D_a", FI);
 
   CodeGenFunction(*this).GenerateCXXGlobalDtorsFunc(Fn, CXXGlobalDtors);
   AddGlobalDtor(Fn);
 }
 
 /// Emit the code necessary to initialize the given global variable.
 void CodeGenFunction::GenerateCXXGlobalVarDeclInitFunc(llvm::Function *Fn,
                                                        const VarDecl *D,
llvm::Function *CodeGenFunction::generateDestroyHelper(
   ImplicitParamDecl dst(getContext(), nullptr, SourceLocation(), nullptr,
                         getContext().VoidPtrTy);
   args.push_back(&dst);
 
   const CGFunctionInfo &FI = CGM.getTypes().arrangeFreeFunctionDeclaration(
       getContext().VoidTy, args, FunctionType::ExtInfo(), /*variadic=*/false);
   llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
   llvm::Function *fn = CGM.CreateGlobalInitOrDestructFunction(
-      FTy, "__cxx_global_array_dtor", VD->getLocation());
+      FTy, "__cxx_global_array_dtor", FI, VD->getLocation());
 
   CurEHLocation = VD->getLocStart();
 
   StartFunction(VD, getContext().VoidTy, fn, FI, args);
 
   emitDestroy(addr, type, destroyer, useEHCleanupForArray);
   
   FinishFunction();
   
   return fn;
 }

cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp

if (CGM.getLangOpts().CPlusPlus && PerformInit) {
                             /*Id=*/nullptr, CGM.getContext().VoidPtrTy);
       Args.push_back(&Dst);
 
       auto &FI = CGM.getTypes().arrangeFreeFunctionDeclaration(
           CGM.getContext().VoidPtrTy, Args, FunctionType::ExtInfo(),
           /*isVariadic=*/false);
       auto FTy = CGM.getTypes().GetFunctionType(FI);
       auto Fn = CGM.CreateGlobalInitOrDestructFunction(
-          FTy, ".__kmpc_global_ctor_.", Loc);
+          FTy, ".__kmpc_global_ctor_.", FI, Loc);
       CtorCGF.StartFunction(GlobalDecl(), CGM.getContext().VoidPtrTy, Fn, FI,
                             Args, SourceLocation());
       auto ArgVal = CtorCGF.EmitLoadOfScalar(
           CtorCGF.GetAddrOfLocalVar(&Dst), /*Volatile=*/false,
           CGM.getContext().VoidPtrTy, Dst.getLocation());
       Address Arg = Address(ArgVal, VDAddr.getAlignment());
       Arg = CtorCGF.Builder.CreateElementBitCast(Arg,
                                              CtorCGF.ConvertTypeForMem(ASTTy));
if (VD->getType().isDestructedType() != QualType::DK_none) {
                             /*Id=*/nullptr, CGM.getContext().VoidPtrTy);
       Args.push_back(&Dst);
 
       auto &FI = CGM.getTypes().arrangeFreeFunctionDeclaration(
           CGM.getContext().VoidTy, Args, FunctionType::ExtInfo(),
           /*isVariadic=*/false);
       auto FTy = CGM.getTypes().GetFunctionType(FI);
       auto Fn = CGM.CreateGlobalInitOrDestructFunction(
-          FTy, ".__kmpc_global_dtor_.", Loc);
+          FTy, ".__kmpc_global_dtor_.", FI, Loc);
       DtorCGF.StartFunction(GlobalDecl(), CGM.getContext().VoidTy, Fn, FI, Args,
                             SourceLocation());
       auto ArgVal = DtorCGF.EmitLoadOfScalar(
           DtorCGF.GetAddrOfLocalVar(&Dst),
           /*Volatile=*/false, CGM.getContext().VoidPtrTy, Dst.getLocation());
       DtorCGF.emitDestroy(Address(ArgVal, VDAddr.getAlignment()), ASTTy,
                           DtorCGF.getDestroyer(ASTTy.isDestructedType()),
                           DtorCGF.needsEHCleanup(ASTTy.isDestructedType()));
if (Dtor == nullptr) {
       auto DtorTy = llvm::FunctionType::get(CGM.VoidTy, CGM.VoidPtrTy,
                                             /*isVarArg=*/false)->getPointerTo();
       Dtor = llvm::Constant::getNullValue(DtorTy);
     }
     if (!CGF) {
       auto InitFunctionTy =
           llvm::FunctionType::get(CGM.VoidTy, /*isVarArg*/ false);
       auto InitFunction = CGM.CreateGlobalInitOrDestructFunction(
-          InitFunctionTy, ".__omp_threadprivate_init_.");
+          InitFunctionTy, ".__omp_threadprivate_init_.",
+          CGM.getTypes().arrangeNullaryFunction());
       CodeGenFunction InitCGF(CGM);
       FunctionArgList ArgList;
       InitCGF.StartFunction(GlobalDecl(), CGM.getContext().VoidTy, InitFunction,
                             CGM.getTypes().arrangeNullaryFunction(), ArgList,
                             Loc);
       emitThreadPrivateVarInit(InitCGF, VDAddr, Ctor, CopyCtor, Dtor, Loc);
       InitCGF.FinishFunction();
       return InitFunction;

cfe/trunk/lib/CodeGen/CodeGenModule.h

public:
   /// will be created and all uses of the old variable will be replaced with a
   /// bitcast to the new variable.
   llvm::GlobalVariable *
   CreateOrReplaceCXXRuntimeVariable(StringRef Name, llvm::Type *Ty,
                                     llvm::GlobalValue::LinkageTypes Linkage);
 
   llvm::Function *
   CreateGlobalInitOrDestructFunction(llvm::FunctionType *ty, const Twine &name,
+                                     const CGFunctionInfo &FI,
                                      SourceLocation Loc = SourceLocation(),
                                      bool TLS = false);
 
   /// Return the address space of the underlying global variable for D, as
   /// determined by its declaration. Normally this is the same as the address
   /// space of D's type, but in CUDA, address spaces are associated with
   /// declarations, not types.
   unsigned GetGlobalVarAddressSpace(const VarDecl *D, unsigned AddrSpace);

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

void ItaniumCXXABI::EmitThreadLocalInitFuncs(
     ArrayRef<std::pair<const VarDecl *, llvm::GlobalVariable *>>
         CXXThreadLocals, ArrayRef<llvm::Function *> CXXThreadLocalInits,
     ArrayRef<llvm::GlobalVariable *> CXXThreadLocalInitVars) {
   llvm::Function *InitFunc = nullptr;
   if (!CXXThreadLocalInits.empty()) {
     // Generate a guarded initialization function.
     llvm::FunctionType *FTy =
         llvm::FunctionType::get(CGM.VoidTy, /*isVarArg=*/false);
-    InitFunc = CGM.CreateGlobalInitOrDestructFunction(FTy, "__tls_init",
+    const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
+    InitFunc = CGM.CreateGlobalInitOrDestructFunction(FTy, "__tls_init", FI,
                                                       SourceLocation(),
                                                       /*TLS=*/true);
     llvm::GlobalVariable *Guard = new llvm::GlobalVariable(
         CGM.getModule(), CGM.Int8Ty, /*isConstant=*/false,
         llvm::GlobalVariable::InternalLinkage,
         llvm::ConstantInt::get(CGM.Int8Ty, 0), "__tls_guard");
     Guard->setThreadLocal(true);
 

cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp

for (size_t I = 0, E = CXXThreadLocalInitVars.size(); I != E; ++I) {
     else
       NonComdatInits.push_back(F);
   }
 
   if (!NonComdatInits.empty()) {
     llvm::FunctionType *FTy =
         llvm::FunctionType::get(CGM.VoidTy, /*isVarArg=*/false);
     llvm::Function *InitFunc = CGM.CreateGlobalInitOrDestructFunction(
-        FTy, "__tls_init", SourceLocation(),
-        /*TLS=*/true);
+        FTy, "__tls_init", CGM.getTypes().arrangeNullaryFunction(),
+        SourceLocation(), /*TLS=*/true);
     CodeGenFunction(CGM).GenerateCXXGlobalInitFunc(InitFunc, NonComdatInits);
 
     AddToXDU(InitFunc);
   }
 }
 
 LValue MicrosoftCXXABI::EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF,
                                                      const VarDecl *VD,

cfe/trunk/test/CodeGenCXX/funcattrs-global-ctor-dtor.cpp

+// RUN: %clang_cc1 %s -triple x86_64-apple-darwin -S -stack-protector 2 -emit-llvm -o - | FileCheck %s
+
+class A {
+ public:
+  virtual ~A() {}
+};
+
+A g;
+
+// CHECK: define internal void @__cxx_global_var_init() [[ATTR0:#[0-9]+]]
+// CHECK: define internal void @_GLOBAL__sub_I_funcattrs_global_ctor_dtor.cpp() [[ATTR0]]
+// CHECK: attributes [[ATTR0]] = {{{.*}} sspstrong {{.*}}}