From 3fa8a2e3a663e55cc46eae57ecfb785ab56852c8 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 27 Feb 2022 00:21:45 +0100 Subject: [PATCH] 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. --- gen/llvmhelpers.cpp | 41 +++++++++++++++++++------------- gen/llvmhelpers.h | 3 +-- gen/tollvm.cpp | 8 +++++-- ir/iraggr.cpp | 12 +--------- ir/irvar.cpp | 8 ++----- tests/codegen/dllimport_gh3916.d | 12 ++++++++++ 6 files changed, 47 insertions(+), 37 deletions(-) create mode 100644 tests/codegen/dllimport_gh3916.d diff --git a/gen/llvmhelpers.cpp b/gen/llvmhelpers.cpp index 15c18721d04..23cfe2ab3f6 100644 --- a/gen/llvmhelpers.cpp +++ b/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, diff --git a/gen/llvmhelpers.h b/gen/llvmhelpers.h index 5f4aa815b2d..6fe347df440 100644 --- a/gen/llvmhelpers.h +++ b/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 diff --git a/gen/tollvm.cpp b/gen/tollvm.cpp index 2c31918652a..92f3f5ad662 100644 --- a/gen/tollvm.cpp +++ b/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 diff --git a/ir/iraggr.cpp b/ir/iraggr.cpp index 6be580168ea..9d25fd4b5aa 100644 --- a/ir/iraggr.cpp +++ b/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); } ////////////////////////////////////////////////////////////////////////////// diff --git a/ir/irvar.cpp b/ir/irvar.cpp index 6885f6117b4..6d99528f032 100644 --- a/ir/irvar.cpp +++ b/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); } } diff --git a/tests/codegen/dllimport_gh3916.d b/tests/codegen/dllimport_gh3916.d new file mode 100644 index 00000000000..c4f1dc1f151 --- /dev/null +++ b/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); +}