Merge pull request #3923 from kinke/fix3916

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

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) {
+bool defineOnDeclare(Dsymbol* sym, bool) {
-  if (global.params.linkonceTemplates)
+  return global.params.linkonceTemplates && sym->isInstantiated();
-    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 dllimportDataSymbol(Dsymbol *sym) {
-  return sym->isExport() || global.params.dllimport == DLLImport::all ||
+  if (!global.params.targetTriple->isOSWindows())
-         (global.params.dllimport == DLLImport::defaultLibsOnly &&
+    return false;
-          // exclude instantiated symbols from druntime/Phobos templates (see
+
-          // `defineOnDeclare()`)
+  if (sym->isExport() || global.params.dllimport == DLLImport::all ||
-          !sym->isInstantiated() && isDefaultLibSymbol(sym));
+      (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
+/// Indicates whether the specified data symbol is to be declared as dllimport.
-/// candidate.
 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
+    // Also export (non-linkonce_odr) symbols
-    if (!isExported && global.params.dllexport && !hasHiddenUDA) {
+    // * 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 dllimportDataSymbol(aggrdecl);
-    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;
 }
 
 //////////////////////////////////////////////////////////////////////////////

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))) {
+      useDLLImport =
-        const bool isDefinedInRootModule =
+          (V->isExport() || V->linkage == LINK::d) && dllimportDataSymbol(V);
-            !(V->storage_class & STCextern) && !V->inNonRoot();
-        if (!isDefinedInRootModule)
-          useDLLImport = true;
-      }
     }
   }
 

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);
+}