Fix #3916 - undefined symbols with -dllimport=all on Windows

Instantiated data symbols were previously never dllimported with
`-dllimport=all`. So if the parent template instance wasn't
codegen'd into the binary directly, it remained undefined.

For `-dllimport=defaultLibsOnly`, the 'solution' to this problem
was to define-on-declare data symbols instantiated from druntime/
Phobos templates, making sure each binary defines all such symbols
it references.

In both cases, switch to an approach where we dllimport all
instantiated data symbols (or druntime/Phobos symbols only), and
dllexport them whenever defining them (so that other object files
or binaries can import them). This may lead to more 'importing
locally defined symbol' linker warnings, but may also lead to less
duplicates and possibly 'proper' sharing of instantiated globals
across the whole process.

This is superfluous and skipped with `-linkonce-templates`, as
that mode defines all referenced instantiated symbols in each
binary anyway, and so has already been a workaround.

Author: kinke

gen/llvmhelpers.cpp

@@ -1607,7 +1607,7 @@ DValue *DtoSymbolAddress(const Loc &loc, Type *type, Declaration *decl) {
   if (SymbolDeclaration *sdecl = decl->isSymbolDeclaration()) {
     // this is the static initialiser (init symbol) for aggregates
     AggregateDeclaration *ad = sdecl->dsym;
-    IF_LOG Logger::print("Sym: ad=%s\n", ad->toChars());
+    IF_LOG Logger::print("init symbol of %s\n", ad->toChars());
     DtoResolveDsymbol(ad);
     auto sd = ad->isStructDeclaration();
 
@@ -1733,24 +1733,33 @@ static bool isDefaultLibSymbol(Dsymbol *sym) {
             (md->packages.length > 1 && md->packages.ptr[1] == Id::io)));
 }
 
-bool defineOnDeclare(Dsymbol* sym, bool isFunction) {
-  if (global.params.linkonceTemplates)
-    return sym->isInstantiated();
-
-  // With -dllimport=defaultLibsOnly, an instantiated data symbol from a
-  // druntime/Phobos template may be assigned to an arbitrary binary (and culled
-  // from others via `needsCodegen()`). Define it in each referencing CU and
-  // never dllimport.
-  return !isFunction && global.params.dllimport == DLLImport::defaultLibsOnly &&
-         sym->isInstantiated() && isDefaultLibSymbol(sym);
+bool defineOnDeclare(Dsymbol* sym, bool) {
+  return global.params.linkonceTemplates && sym->isInstantiated();
 }
 
 bool dllimportDataSymbol(Dsymbol *sym) {
-  return sym->isExport() || global.params.dllimport == DLLImport::all ||
-         (global.params.dllimport == DLLImport::defaultLibsOnly &&
-          // exclude instantiated symbols from druntime/Phobos templates (see
-          // `defineOnDeclare()`)
-          !sym->isInstantiated() && isDefaultLibSymbol(sym));
+  if (!global.params.targetTriple->isOSWindows())
+    return false;
+
+  if (sym->isExport() || global.params.dllimport == DLLImport::all ||
+      (global.params.dllimport == DLLImport::defaultLibsOnly &&
+       isDefaultLibSymbol(sym))) {
+    // Okay, this symbol is a candidate. Use dllimport unless we have a
+    // guaranteed-codegen'd definition in a root module.
+    if (auto mod = sym->isModule()) {
+      return !mod->isRoot(); // non-root ModuleInfo symbol
+    } else if (sym->inNonRoot()) {
+      return true; // not instantiated, and defined in non-root
+    } else if (!global.params.linkonceTemplates &&
+               sym->isInstantiated()) {
+      return true; // instantiated but potentially culled (needsCodegen())
+    } else if (auto vd = sym->isVarDeclaration()) {
+      if (vd->storage_class & STCextern)
+        return true; // externally defined global variable
+    }
+  }
+
+  return false;
 }
 
 llvm::GlobalVariable *declareGlobal(const Loc &loc, llvm::Module &module,

gen/llvmhelpers.h

@@ -247,8 +247,7 @@ llvm::Constant *buildStringLiteralConstant(StringExp *se, bool zeroTerm);
 /// primarily for -linkonce-templates.
 bool defineOnDeclare(Dsymbol *sym, bool isFunction);
 
-/// Indicates whether the specified data symbol is a general dllimport
-/// candidate.
+/// Indicates whether the specified data symbol is to be declared as dllimport.
 bool dllimportDataSymbol(Dsymbol *sym);
 
 /// Tries to declare an LLVM global. If a variable with the same mangled name

gen/tollvm.cpp

@@ -290,8 +290,12 @@ void setVisibility(Dsymbol *sym, llvm::GlobalObject *obj) {
 
   if (triple.isOSWindows()) {
     bool isExported = sym->isExport();
-    // also export with -fvisibility=public without @hidden
-    if (!isExported && global.params.dllexport && !hasHiddenUDA) {
+    // Also export (non-linkonce_odr) symbols
+    // * with -fvisibility=public without @hidden, or
+    // * if declared with dllimport (so potentially imported from other object
+    //   files / DLLs).
+    if (!isExported && ((global.params.dllexport && !hasHiddenUDA) ||
+                        obj->hasDLLImportStorageClass())) {
       isExported = hasExportedLinkage(obj);
     }
     // reset default visibility & DSO locality - on Windows, the DLL storage

ir/iraggr.cpp

@@ -53,17 +53,7 @@ bool IrAggr::suppressTypeInfo() const {
 //////////////////////////////////////////////////////////////////////////////
 
 bool IrAggr::useDLLImport() const {
-  if (!global.params.targetTriple->isOSWindows())
-    return false;
-
-  if (dllimportDataSymbol(aggrdecl)) {
-    // dllimport, unless defined in a root module (=> no extra indirection for
-    // other root modules, assuming *all* root modules will be linked together
-    // to one or more binaries).
-    return aggrdecl->inNonRoot();
-  }
-
-  return false;
+  return dllimportDataSymbol(aggrdecl);
 }
 
 //////////////////////////////////////////////////////////////////////////////

ir/irvar.cpp

@@ -86,12 +86,8 @@ void IrGlobal::declare() {
     // dllimport isn't supported for thread-local globals (MSVC++ neither)
     if (!V->isThreadlocal()) {
       // implicitly include extern(D) globals with -dllimport
-      if (V->isExport() || (V->linkage == LINK::d && dllimportDataSymbol(V))) {
-        const bool isDefinedInRootModule =
-            !(V->storage_class & STCextern) && !V->inNonRoot();
-        if (!isDefinedInRootModule)
-          useDLLImport = true;
-      }
+      useDLLImport =
+          (V->isExport() || V->linkage == LINK::d) && dllimportDataSymbol(V);
     }
   }
 

tests/codegen/dllimport_gh3916.d

@@ -0,0 +1,12 @@
+// REQUIRES: Windows
+
+// RUN: %ldc -output-ll -dllimport=all             -of=%t_all.ll %s && FileCheck %s < %t_all.ll
+// RUN: %ldc -output-ll -dllimport=defaultLibsOnly -of=%t_dlo.ll %s && FileCheck %s < %t_dlo.ll
+
+import std.random : Xorshift; // pre-instantiated template
+
+void foo()
+{
+    // CHECK: _D3std6random__T14XorshiftEngine{{.*}}6__initZ = external dllimport
+    const i = __traits(initSymbol, Xorshift);
+}