Split up getOrCreateGlobal() in {declare,define}Global()

Making it obvious which of the two operations is performed, reducing
call args and making sure a global isn't defined multiple times via
`defineGlobal()`.

The only [intended] functional change is in gen/trycatchfinally.cpp,
where I inserted a check for an existing __cpp_type_info_ptr global when
emitting a catch for C++ exceptions.

gen/llvmhelpers.cpp

@@ -911,17 +911,12 @@ void DtoResolveVariable(VarDeclaration *vd) {
     // with a different type later, swap it out and replace any existing
     // uses with bitcasts to the previous type.
 
-    // We always start out with external linkage; any other type is set
-    // when actually defining it in VarDeclaration::codegen.
-    llvm::GlobalValue::LinkageTypes linkage =
-        llvm::GlobalValue::ExternalLinkage;
-    if (vd->llvmInternal == LLVMextern_weak) {
-      linkage = llvm::GlobalValue::ExternalWeakLinkage;
-    }
-
     llvm::GlobalVariable *gvar =
-        getOrCreateGlobal(vd->loc, gIR->module, DtoMemType(vd->type), isLLConst,
+        declareGlobal(vd->loc, gIR->module, DtoMemType(vd->type), irMangle,
-                          linkage, nullptr, irMangle, vd->isThreadlocal());
+                      isLLConst, vd->isThreadlocal());
+    if (vd->llvmInternal == LLVMextern_weak)
+      gvar->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
+
     auto varIr = getIrGlobal(vd);
     varIr->value = gvar;
 
@@ -1751,19 +1746,20 @@ llvm::Constant *buildStringLiteralConstant(StringExp *se, bool zeroTerm) {
   return LLConstantArray::get(at, vals);
 }
 
-llvm::GlobalVariable *getOrCreateGlobal(const Loc &loc, llvm::Module &module,
+llvm::GlobalVariable *declareGlobal(const Loc &loc, llvm::Module &module,
-                                        llvm::Type *type, bool isConstant,
+                                    llvm::Type *type,
-                                        llvm::GlobalValue::LinkageTypes linkage,
+                                    llvm::StringRef mangledName,
-                                        llvm::Constant *init,
+                                    bool isConstant, bool isThreadLocal) {
-                                        llvm::StringRef name,
+  llvm::GlobalVariable *existing =
-                                        bool isThreadLocal) {
+      module.getGlobalVariable(mangledName, /*AllowInternal=*/true);
-  llvm::GlobalVariable *existing = module.getGlobalVariable(name, true);
   if (existing) {
-    if (existing->getType()->getElementType() != type) {
+    if (existing->getType()->getElementType() != type ||
+        existing->isConstant() != isConstant ||
+        existing->isThreadLocal() != isThreadLocal) {
       error(loc,
             "Global variable type does not match previous declaration with "
             "same mangled name: `%s`",
-            name.str().c_str());
+            mangledName.str().c_str());
       fatal();
     }
     return existing;
@@ -1772,14 +1768,36 @@ llvm::GlobalVariable *getOrCreateGlobal(const Loc &loc, llvm::Module &module,
   // Use a command line option for the thread model.
   // On PPC there is only local-exec available - in this case just ignore the
   // command line.
-  const llvm::GlobalVariable::ThreadLocalMode tlsModel =
+  const auto tlsModel =
       isThreadLocal
           ? (global.params.targetTriple->getArch() == llvm::Triple::ppc
                  ? llvm::GlobalVariable::LocalExecTLSModel
                  : clThreadModel.getValue())
           : llvm::GlobalVariable::NotThreadLocal;
-  return new llvm::GlobalVariable(module, type, isConstant, linkage, init, name,
+
-                                  nullptr, tlsModel);
+  return new llvm::GlobalVariable(module, type, isConstant,
+                                  llvm::GlobalValue::ExternalLinkage, nullptr,
+                                  mangledName, nullptr, tlsModel);
+}
+
+void defineGlobal(llvm::GlobalVariable *global, llvm::Constant *init,
+                  llvm::GlobalValue::LinkageTypes linkage) {
+  assert(global->isDeclaration() && "Global variable already defined");
+  assert(init);
+  global->setInitializer(init);
+  global->setLinkage(linkage);
+}
+
+llvm::GlobalVariable *defineGlobal(const Loc &loc, llvm::Module &module,
+                                   llvm::StringRef mangledName,
+                                   llvm::Constant *init,
+                                   llvm::GlobalValue::LinkageTypes linkage,
+                                   bool isConstant, bool isThreadLocal) {
+  assert(init);
+  auto global = declareGlobal(loc, module, init->getType(), mangledName,
+                              isConstant, isThreadLocal);
+  defineGlobal(global, init, linkage);
+  return global;
 }
 
 FuncDeclaration *getParentFunc(Dsymbol *sym) {

gen/llvmhelpers.h

@@ -245,18 +245,29 @@ stringLiteralCacheForType(Type *charType);
 
 llvm::Constant *buildStringLiteralConstant(StringExp *se, bool zeroTerm);
 
-/// Tries to create an LLVM global with the given properties. If a variable with
+/// Tries to declare an LLVM global. If a variable with the same mangled name
-/// the same mangled name already exists, checks if the types match and returns
+/// already exists, checks if the types match and returns it instead.
-/// it instead.
 ///
 /// Necessary to support multiple declarations with the same mangled name, as
 /// can be the case due to pragma(mangle).
-llvm::GlobalVariable *getOrCreateGlobal(const Loc &loc, llvm::Module &module,
+llvm::GlobalVariable *declareGlobal(const Loc &loc, llvm::Module &module,
-                                        llvm::Type *type, bool isConstant,
+                                    llvm::Type *type,
-                                        llvm::GlobalValue::LinkageTypes linkage,
+                                    llvm::StringRef mangledName,
-                                        llvm::Constant *init,
+                                    bool isConstant,
-                                        llvm::StringRef name,
+                                    bool isThreadLocal = false);
-                                        bool isThreadLocal = false);
+
+/// Defines an existing LLVM global, i.e., sets the initial value and finalizes
+/// its linkage.
+/// Asserts that a global isn't defined multiple times this way.
+void defineGlobal(llvm::GlobalVariable *global, llvm::Constant *init,
+                  llvm::GlobalValue::LinkageTypes linkage);
+
+/// Declares (if not already declared) & defines an LLVM global.
+llvm::GlobalVariable *defineGlobal(const Loc &loc, llvm::Module &module,
+                                   llvm::StringRef mangledName,
+                                   llvm::Constant *init,
+                                   llvm::GlobalValue::LinkageTypes linkage,
+                                   bool isConstant, bool isThreadLocal = false);
 
 FuncDeclaration *getParentFunc(Dsymbol *sym);
 

gen/modules.cpp

@@ -169,18 +169,15 @@ LLFunction *build_module_reference_and_ctor(const char *moduleMangle,
 
   // create the ModuleReference node for this module
   const auto thismrefIRMangle = getIRMangledModuleRefSymbolName(moduleMangle);
-  Loc loc;
+  LLGlobalVariable *thismref =
-  LLGlobalVariable *thismref = getOrCreateGlobal(
+      defineGlobal(Loc(), gIR->module, thismrefIRMangle, thismrefinit,
-      loc, gIR->module, modulerefTy, false, LLGlobalValue::InternalLinkage,
+                   LLGlobalValue::InternalLinkage, false);
-      thismrefinit, thismrefIRMangle);
   // make sure _Dmodule_ref is declared
   const auto mrefIRMangle = getIRMangledVarName("_Dmodule_ref", LINKc);
   LLConstant *mref = gIR->module.getNamedGlobal(mrefIRMangle);
   LLType *modulerefPtrTy = getPtrToType(modulerefTy);
   if (!mref) {
-    mref = new LLGlobalVariable(gIR->module, modulerefPtrTy, false,
+    mref = declareGlobal(Loc(), gIR->module, modulerefPtrTy, mrefIRMangle, false);
-                                LLGlobalValue::ExternalLinkage, nullptr,
-                                mrefIRMangle);
   }
   mref = DtoBitCast(mref, getPtrToType(modulerefPtrTy));
 
@@ -218,9 +215,9 @@ llvm::Function *buildGetTLSAnchor() {
   // Create a dummmy TLS global private to this module.
   const auto one =
       llvm::ConstantInt::get(llvm::Type::getInt8Ty(gIR->context()), 1);
-  const auto anchor = getOrCreateGlobal(
+  const auto anchor = defineGlobal(Loc(), gIR->module, "ldc.tls_anchor", one,
-      Loc(), gIR->module, one->getType(), false,
+                                   llvm::GlobalValue::LinkOnceODRLinkage, false,
-      llvm::GlobalValue::LinkOnceODRLinkage, one, "ldc.tls_anchor", true);
+                                   /*isThreadLocal=*/true);
   anchor->setVisibility(llvm::GlobalValue::HiddenVisibility);
   anchor->setAlignment(16);
 
@@ -358,11 +355,11 @@ void emitModuleRefToSection(RegistryStyle style, std::string moduleMangle,
 
   const auto thismrefIRMangle =
       getIRMangledModuleRefSymbolName(moduleMangle.c_str());
-  auto thismref = new llvm::GlobalVariable(
+  auto thismref = defineGlobal(Loc(), gIR->module, thismrefIRMangle,
-      gIR->module, moduleInfoPtrTy,
+                               DtoBitCast(thisModuleInfo, moduleInfoPtrTy),
-      false, // FIXME: mRelocModel != llvm::Reloc::PIC_
+                               llvm::GlobalValue::LinkOnceODRLinkage,
-      llvm::GlobalValue::LinkOnceODRLinkage,
+                               false // FIXME: mRelocModel != llvm::Reloc::PIC_
-      DtoBitCast(thisModuleInfo, moduleInfoPtrTy), thismrefIRMangle);
+  );
   thismref->setSection(sectionName);
   gIR->usedArray.push_back(thismref);
 
@@ -382,22 +379,19 @@ void emitModuleRefToSection(RegistryStyle style, std::string moduleMangle,
   const auto magicEndSymbolName = (style == RegistryStyle::sectionDarwin)
                                       ? "\1section$end$__DATA$.minfo"
                                       : "__stop___minfo";
-  auto minfoBeg = new llvm::GlobalVariable(gIR->module, moduleInfoPtrTy, false,
+  auto minfoBeg = declareGlobal(Loc(), gIR->module, moduleInfoPtrTy,
-                                           llvm::GlobalValue::ExternalLinkage,
+                                magicBeginSymbolName, false);
-                                           nullptr, magicBeginSymbolName);
+  auto minfoEnd = declareGlobal(Loc(), gIR->module, moduleInfoPtrTy,
-  auto minfoEnd = new llvm::GlobalVariable(gIR->module, moduleInfoPtrTy, false,
+                                magicEndSymbolName, false);
-                                           llvm::GlobalValue::ExternalLinkage,
-                                           nullptr, magicEndSymbolName);
   minfoBeg->setVisibility(llvm::GlobalValue::HiddenVisibility);
   minfoEnd->setVisibility(llvm::GlobalValue::HiddenVisibility);
 
   // Build the ctor to invoke _d_dso_registry.
 
   // This is the DSO slot for use by the druntime implementation.
-  auto dsoSlot =
+  auto dsoSlot = defineGlobal(Loc(), gIR->module, "ldc.dso_slot",
-      new llvm::GlobalVariable(gIR->module, getVoidPtrType(), false,
+                              getNullPtr(getVoidPtrType()),
-                               llvm::GlobalValue::LinkOnceODRLinkage,
+                              llvm::GlobalValue::LinkOnceODRLinkage, false);
-                               getNullPtr(getVoidPtrType()), "ldc.dso_slot");
   dsoSlot->setVisibility(llvm::GlobalValue::HiddenVisibility);
 
   // Okay, so the theory is easy: We want to have one global constructor and
@@ -429,11 +423,10 @@ void emitModuleRefToSection(RegistryStyle style, std::string moduleMangle,
   // problems. This would mean that it is no longer safe to link D objects
   // directly using e.g. "g++ dcode.o cppcode.o", though.
 
-  auto dsoInitialized = new llvm::GlobalVariable(
+  auto dsoInitialized = defineGlobal(
-      gIR->module, llvm::Type::getInt8Ty(gIR->context()), false,
+      Loc(), gIR->module, "ldc.dso_initialized",
-      llvm::GlobalValue::LinkOnceODRLinkage,
       llvm::ConstantInt::get(llvm::Type::getInt8Ty(gIR->context()), 0),
-      "ldc.dso_initialized");
+      llvm::GlobalValue::LinkOnceODRLinkage, false);
   dsoInitialized->setVisibility(llvm::GlobalValue::HiddenVisibility);
 
   // There is no reason for this cast to void*, other than that removing it
@@ -505,7 +498,7 @@ void addCoverageAnalysis(Module *m) {
     llvm::ConstantAggregateZero *zeroinitializer =
         llvm::ConstantAggregateZero::get(type);
     m->d_cover_valid = new llvm::GlobalVariable(
-        gIR->module, type, true, LLGlobalValue::InternalLinkage,
+        gIR->module, type, /*isConstant=*/true, LLGlobalValue::InternalLinkage,
         zeroinitializer, "_d_cover_valid");
     LLConstant *idxs[] = {DtoConstUint(0), DtoConstUint(0)};
     d_cover_valid_slice =

gen/ms-cxx-helper.cpp

@@ -165,9 +165,8 @@ llvm::StructType *getTypeDescriptorType(IRState &irs,
 llvm::GlobalVariable *getTypeDescriptor(IRState &irs, ClassDeclaration *cd) {
   if (cd->isCPPclass()) {
     const char *name = Target::cppTypeInfoMangle(cd);
-    return getOrCreateGlobal(
+    return declareGlobal(cd->loc, irs.module, getVoidPtrType(), name,
-        cd->loc, irs.module, getVoidPtrType(), /*isConstant=*/true,
+                         /*isConstant=*/true);
-        LLGlobalValue::ExternalLinkage, /*init=*/nullptr, name);
   }
 
   auto classInfoPtr = getIrAggr(cd, true)->getClassInfoSymbol();
@@ -188,10 +187,9 @@ llvm::GlobalVariable *getTypeDescriptor(IRState &irs, ClassDeclaration *cd) {
       llvm::ConstantDataArray::getString(gIR->context(), TypeNameString)};
   llvm::StructType *TypeDescriptorType =
       getTypeDescriptorType(irs, classInfoPtr, TypeNameString);
-  Var = new llvm::GlobalVariable(
+  Var = defineGlobal(cd->loc, gIR->module, TypeDescName,
-      gIR->module, TypeDescriptorType, /*Constant=*/false,
+                     llvm::ConstantStruct::get(TypeDescriptorType, Fields),
-      LLGlobalVariable::InternalLinkage, // getLinkageForRTTI(Type),
+                     LLGlobalVariable::InternalLinkage, false);
-      llvm::ConstantStruct::get(TypeDescriptorType, Fields), TypeDescName);
   return Var;
 }
 

gen/trycatchfinally.cpp

@@ -161,31 +161,36 @@ void TryCatchScope::emitCatchBodies(IRState &irs, llvm::Value *ehPtrSlot) {
 
     LLGlobalVariable *ci;
     if (p.cd->isCPPclass()) {
-      const char *name = Target::cppTypeInfoMangle(p.cd);
+      // Wrap std::type_info pointers inside a __cpp_type_info_ptr class
-      auto cpp_ti = getOrCreateGlobal(
+      // instance so that the personality routine may differentiate C++ catch
-          p.cd->loc, irs.module, getVoidPtrType(), /*isConstant=*/true,
+      // clauses from D ones.
-          LLGlobalValue::ExternalLinkage, /*init=*/nullptr, name);
+      OutBuffer wrapperMangleBuf;
+      wrapperMangleBuf.writestring("_D");
+      mangleToBuffer(p.cd, &wrapperMangleBuf);
+      wrapperMangleBuf.printf("%d%s", 18, "_cpp_type_info_ptr");
+      const auto wrapperMangle =
+          getIRMangledVarName(wrapperMangleBuf.peekString(), LINKd);
 
-      // Wrap std::type_info pointers inside a __cpp_type_info_ptr class instance so that
+      ci = irs.module.getGlobalVariable(wrapperMangle);
-      // the personality routine may differentiate C++ catch clauses from D ones.
+      if (!ci) {
-      OutBuffer mangleBuf;
+        const char *name = Target::cppTypeInfoMangle(p.cd);
-      mangleBuf.writestring("_D");
+        auto cpp_ti =
-      mangleToBuffer(p.cd, &mangleBuf);
+            declareGlobal(p.cd->loc, irs.module, getVoidPtrType(), name,
-      mangleBuf.printf("%d%s", 18, "_cpp_type_info_ptr");
+                          /*isConstant=*/true);
-      const auto wrapperMangle = getIRMangledVarName(mangleBuf.peekString(), LINKd);
 
-      const auto cppTypeInfoPtrType = getCppTypeInfoPtrType();
+        const auto cppTypeInfoPtrType = getCppTypeInfoPtrType();
-      RTTIBuilder b(cppTypeInfoPtrType);
+        RTTIBuilder b(cppTypeInfoPtrType);
-      b.push(cpp_ti);
+        b.push(cpp_ti);
 
-      auto wrapperType = llvm::cast<llvm::StructType>(
+        auto wrapperType = llvm::cast<llvm::StructType>(
-          static_cast<IrTypeClass *>(cppTypeInfoPtrType->ctype)
+            static_cast<IrTypeClass *>(cppTypeInfoPtrType->ctype)
-              ->getMemoryLLType());
+                ->getMemoryLLType());
-      auto wrapperInit = b.get_constant(wrapperType);
+        auto wrapperInit = b.get_constant(wrapperType);
 
-      ci = getOrCreateGlobal(
+        ci = defineGlobal(p.cd->loc, irs.module, wrapperMangle, wrapperInit,
-          p.cd->loc, irs.module, wrapperType, /*isConstant=*/true,
+                          LLGlobalValue::LinkOnceODRLinkage,
-          LLGlobalValue::LinkOnceODRLinkage, wrapperInit, wrapperMangle);
+                          /*isConstant=*/true);
+      }
     } else {
       ci = getIrAggr(p.cd)->getClassInfoSymbol();
     }

gen/typinf.cpp

@@ -624,18 +624,11 @@ void TypeInfoDeclaration_codegen(TypeInfoDeclaration *decl, IRState *p) {
   }
 
   const auto irMangle = getIRMangledVarName(mangled, LINKd);
-  LLGlobalVariable *gvar = gIR->module.getGlobalVariable(irMangle);
+  LLType *type = DtoType(decl->type)->getPointerElementType();
-  if (gvar) {
+  // Declare the symbol. We need to keep it mutable as the type is not
-    assert(gvar->getType()->getContainedType(0)->isStructTy());
+  // declared as immutable on the D side, and e.g. synchronized() can be used
-  } else {
+  // on the implicit monitor.
-    LLType *type = DtoType(decl->type)->getPointerElementType();
+  auto gvar = declareGlobal(decl->loc, gIR->module, type, irMangle, false);
-    // Create the symbol. We need to keep it mutable as the type is not declared
-    // as immutable on the D side, and e.g. synchronized() can be used on the
-    // implicit monitor.
-    gvar =
-        new LLGlobalVariable(gIR->module, type, false,
-                             LLGlobalValue::ExternalLinkage, nullptr, irMangle);
-  }
 
   IrGlobal *irg = getIrGlobal(decl, true);
   irg->value = gvar;

ir/iraggr.cpp

@@ -49,9 +49,8 @@ LLConstant *&IrAggr::getInitSymbol() {
   // create the initZ symbol
   const auto irMangle = getIRMangledInitSymbolName(aggrdecl);
 
-  auto initGlobal =
+  auto initGlobal = declareGlobal(aggrdecl->loc, gIR->module, getLLStructType(),
-      getOrCreateGlobal(aggrdecl->loc, gIR->module, getLLStructType(), true,
+                                  irMangle, /*isConstant=*/true);
-                        llvm::GlobalValue::ExternalLinkage, nullptr, irMangle);
   initGlobal->setAlignment(DtoAlignment(type));
 
   init = initGlobal;

ir/irclass.cpp

@@ -51,9 +51,8 @@ LLGlobalVariable *IrAggr::getVtblSymbol() {
 
   LLType *vtblTy = stripModifiers(type)->ctype->isClass()->getVtblType();
 
-  vtbl =
+  vtbl = declareGlobal(aggrdecl->loc, gIR->module, vtblTy, irMangle,
-      getOrCreateGlobal(aggrdecl->loc, gIR->module, vtblTy, true,
+                       /*isConstant=*/true);
-                        llvm::GlobalValue::ExternalLinkage, nullptr, irMangle);
 
   return vtbl;
 }
@@ -77,9 +76,8 @@ LLGlobalVariable *IrAggr::getClassInfoSymbol() {
   assert(tc && "invalid ClassInfo type");
 
   // classinfos cannot be constants since they're used as locks for synchronized
-  classInfo = getOrCreateGlobal(
+  classInfo = declareGlobal(aggrdecl->loc, gIR->module, tc->getMemoryLLType(),
-      aggrdecl->loc, gIR->module, tc->getMemoryLLType(), false,
+                            irMangle, false);
-      llvm::GlobalValue::ExternalLinkage, nullptr, irMangle);
 
   // Generate some metadata on this ClassInfo if it's for a class.
   ClassDeclaration *classdecl = aggrdecl->isClassDeclaration();
@@ -139,11 +137,8 @@ LLGlobalVariable *IrAggr::getInterfaceArraySymbol() {
 
   LLArrayType *array_type = llvm::ArrayType::get(InterfaceTy, n);
 
-  // We keep the global as external for now and only consider template linkage
+  classInterfacesArray = declareGlobal(cd->loc, gIR->module, array_type,
-  // if we emit the initializer later.
+                                       irMangle, /*isConstant=*/true);
-  classInterfacesArray =
-      getOrCreateGlobal(cd->loc, gIR->module, array_type, true,
-                        llvm::GlobalValue::ExternalLinkage, nullptr, irMangle);
 
   return classInterfacesArray;
 }
@@ -457,8 +452,8 @@ llvm::GlobalVariable *IrAggr::getInterfaceVtbl(BaseClass *b, bool new_instance,
 
   const auto lwc = DtoLinkage(cd);
   LLGlobalVariable *GV =
-      getOrCreateGlobal(cd->loc, gIR->module, vtbl_constant->getType(), true,
+      defineGlobal(cd->loc, gIR->module, irMangle, vtbl_constant,
-                        lwc.first, vtbl_constant, irMangle);
+                   lwc.first, /*isConstant=*/true);
   setLinkage(lwc, GV);
 
   // insert into the vtbl map

ir/irmodule.cpp

@@ -11,6 +11,7 @@
 #include "module.h"
 #include "gen/llvm.h"
 #include "gen/irstate.h"
+#include "gen/llvmhelpers.h"
 #include "gen/mangling.h"
 #include "gen/tollvm.h"
 #include "ir/irdsymbol.h"
@@ -25,9 +26,9 @@ llvm::GlobalVariable *IrModule::moduleInfoSymbol() {
 
   const auto irMangle = getIRMangledModuleInfoSymbolName(M);
 
-  moduleInfoVar = new llvm::GlobalVariable(
+  moduleInfoVar =
-      gIR->module, llvm::StructType::create(gIR->context()), false,
+      declareGlobal(Loc(), gIR->module,
-      llvm::GlobalValue::ExternalLinkage, nullptr, irMangle);
+                    llvm::StructType::create(gIR->context()), irMangle, false);
   return moduleInfoVar;
 }