From f1f1909a68ca78740a19537d1db966713b558efb Mon Sep 17 00:00:00 2001 From: Alexandre Ganea Date: Fri, 4 Apr 2025 16:03:05 -0400 Subject: [PATCH 1/3] [LLD][COFF] Don't dllimport from static libraries This reverts commit 6a1bdd9 and re-instate behavior that matches what MSVC link.exe does, that is error out when trying to dllimport a symbol from a static library. We also now hint in stdout that we should rather dllimport the symbol from a import library. Fixes https://github.com/llvm/llvm-project/issues/131807 --- lld/COFF/Driver.cpp | 6 ++--- lld/COFF/SymbolTable.cpp | 20 ++++++++++------- lld/COFF/SymbolTable.h | 5 +---- lld/test/COFF/imports-static-lib.test | 32 +++++++++++++++++++++++++++ lld/test/COFF/undefined_lazy.test | 26 ---------------------- 5 files changed, 47 insertions(+), 42 deletions(-) create mode 100644 lld/test/COFF/imports-static-lib.test delete mode 100644 lld/test/COFF/undefined_lazy.test diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp index 9bbab524b1f9a..7aa13bdce488e 100644 --- a/lld/COFF/Driver.cpp +++ b/lld/COFF/Driver.cpp @@ -2663,10 +2663,8 @@ void LinkerDriver::linkerMain(ArrayRef argsArr) { createECExportThunks(); // Resolve remaining undefined symbols and warn about imported locals. - ctx.forEachSymtab([&](SymbolTable &symtab) { - while (symtab.resolveRemainingUndefines()) - run(); - }); + ctx.forEachSymtab( + [&](SymbolTable &symtab) { symtab.resolveRemainingUndefines(); }); if (errorCount()) return; diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp index 16afcc64316e3..d1fb6e05a0320 100644 --- a/lld/COFF/SymbolTable.cpp +++ b/lld/COFF/SymbolTable.cpp @@ -231,6 +231,17 @@ void SymbolTable::reportUndefinedSymbol(const UndefinedDiag &undefDiag) { } if (numDisplayedRefs < numRefs) diag << "\n>>> referenced " << numRefs - numDisplayedRefs << " more times"; + + // Hints + StringRef name = undefDiag.sym->getName(); + if (name.starts_with("__imp_")) { + Symbol *imp = find(name.substr(strlen("__imp_"))); + if (imp && imp->isLazy()) { + diag << "\nNOTE: a relevant symbol '" << imp->getName() + << "' is available in " << toString(imp->getFile()) + << " but cannot be used because it is not a import library."; + } + } } void SymbolTable::loadMinGWSymbols() { @@ -432,11 +443,10 @@ void SymbolTable::reportUnresolvable() { reportProblemSymbols(undefs, /*localImports=*/nullptr, true); } -bool SymbolTable::resolveRemainingUndefines() { +void SymbolTable::resolveRemainingUndefines() { llvm::TimeTraceScope timeScope("Resolve remaining undefined symbols"); SmallPtrSet undefs; DenseMap localImports; - bool foundLazy = false; for (auto &i : symMap) { Symbol *sym = i.second; @@ -481,11 +491,6 @@ bool SymbolTable::resolveRemainingUndefines() { imp = findLocalSym(*mangledName); } } - if (imp && imp->isLazy()) { - forceLazy(imp); - foundLazy = true; - continue; - } if (imp && isa(imp)) { auto *d = cast(imp); replaceSymbol(sym, ctx, name, d); @@ -513,7 +518,6 @@ bool SymbolTable::resolveRemainingUndefines() { reportProblemSymbols( undefs, ctx.config.warnLocallyDefinedImported ? &localImports : nullptr, false); - return foundLazy; } std::pair SymbolTable::insert(StringRef name) { diff --git a/lld/COFF/SymbolTable.h b/lld/COFF/SymbolTable.h index da19bf2800ecf..15e2644a6f519 100644 --- a/lld/COFF/SymbolTable.h +++ b/lld/COFF/SymbolTable.h @@ -67,10 +67,7 @@ class SymbolTable { // Try to resolve any undefined symbols and update the symbol table // accordingly, then print an error message for any remaining undefined // symbols and warn about imported local symbols. - // Returns whether more files might need to be linked in to resolve lazy - // symbols, in which case the caller is expected to call the function again - // after linking those files. - bool resolveRemainingUndefines(); + void resolveRemainingUndefines(); // Load lazy objects that are needed for MinGW automatic import and for // doing stdcall fixups. diff --git a/lld/test/COFF/imports-static-lib.test b/lld/test/COFF/imports-static-lib.test new file mode 100644 index 0000000000000..63539cab9d860 --- /dev/null +++ b/lld/test/COFF/imports-static-lib.test @@ -0,0 +1,32 @@ +# REQUIRES: x86 + +# Ensure that we don't import dllimport symbols from static (non-import) libraries +# RUN: split-file %s %t.dir +# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/foo.s -o %t.foo.obj +# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/main.s -o %t.main.obj +# RUN: llvm-lib %t.foo.obj -out:%t.foo.lib +# RUN: not lld-link %t.foo.lib %t.main.obj -out:%t.dll -dll 2>&1 | FileCheck %s + +CHECK: error: undefined symbol: __declspec(dllimport) foo + +# Now do the same thing, but import the symbol from a import library. +# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/foo_dll_main.s -o %t.foo_dll_main.obj +# RUN: lld-link /out:%t.dll /dll %t.foo.obj %t.foo_dll_main.obj /export:foo /implib:%t.foo.imp.lib +# RUN: lld-link %t.main.obj %t.foo.imp.lib -out:%t.exe -dll + +#--- foo.s +.text +.globl foo +foo: + ret +#--- foo_dll_main.s +.text +.global _DllMainCRTStartup +_DllMainCRTStartup: + ret +#--- main.s +.text +.global _DllMainCRTStartup +_DllMainCRTStartup: + call *__imp_foo(%rip) + ret diff --git a/lld/test/COFF/undefined_lazy.test b/lld/test/COFF/undefined_lazy.test deleted file mode 100644 index ed5cd358b5cd9..0000000000000 --- a/lld/test/COFF/undefined_lazy.test +++ /dev/null @@ -1,26 +0,0 @@ -# REQUIRES: x86 - -# RUN: split-file %s %t.dir -# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/foo.s -o %t.foo.obj -# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/bar.s -o %t.bar.obj -# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/qux.s -o %t.qux.obj -# RUN: llvm-lib %t.foo.obj -out:%t.foo.lib -# RUN: llvm-lib %t.bar.obj -out:%t.bar.lib -# RUN: lld-link %t.foo.lib %t.bar.lib %t.qux.obj -out:%t.dll -dll -# -#--- foo.s -.text -.globl foo -foo: - call bar -#--- bar.s -.text -.globl bar -bar: - ret -#--- qux.s -.text -.global _DllMainCRTStartup -_DllMainCRTStartup: - call *__imp_foo(%rip) - ret From 593bfe8d895459a9b1b6c4924b1cb28fbde996cc Mon Sep 17 00:00:00 2001 From: Alexandre Ganea Date: Fri, 4 Apr 2025 17:54:14 -0400 Subject: [PATCH 2/3] Simplify & add a test case --- lld/COFF/SymbolTable.cpp | 6 ++--- .../COFF/imports-static-lib-indirect.test | 26 +++++++++++++++++++ lld/test/COFF/imports-static-lib.test | 1 + 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 lld/test/COFF/imports-static-lib-indirect.test diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp index d1fb6e05a0320..8fb0ee4e890d6 100644 --- a/lld/COFF/SymbolTable.cpp +++ b/lld/COFF/SymbolTable.cpp @@ -234,12 +234,12 @@ void SymbolTable::reportUndefinedSymbol(const UndefinedDiag &undefDiag) { // Hints StringRef name = undefDiag.sym->getName(); - if (name.starts_with("__imp_")) { - Symbol *imp = find(name.substr(strlen("__imp_"))); + if (name.consume_front("__imp_")) { + Symbol *imp = find(name); if (imp && imp->isLazy()) { diag << "\nNOTE: a relevant symbol '" << imp->getName() << "' is available in " << toString(imp->getFile()) - << " but cannot be used because it is not a import library."; + << " but cannot be used because it is not an import library."; } } } diff --git a/lld/test/COFF/imports-static-lib-indirect.test b/lld/test/COFF/imports-static-lib-indirect.test new file mode 100644 index 0000000000000..0ecb74a13de69 --- /dev/null +++ b/lld/test/COFF/imports-static-lib-indirect.test @@ -0,0 +1,26 @@ +# REQUIRES: x86 + +# Pulling in on both a dllimport symbol and a static symbol should only warn. +# RUN: split-file %s %t.dir +# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/other.s -o %t.other.obj +# RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/main.s -o %t.main.obj +# RUN: llvm-lib %t.other.obj -out:%t.other.lib +# RUN: lld-link %t.other.lib %t.main.obj -out:%t.dll -dll 2>&1 | FileCheck %s + +CHECK: warning: {{.*}} [LNK4217] + +#--- other.s +.text +.globl other +.globl foo +other: + ret +foo: + ret +#--- main.s +.text +.global _DllMainCRTStartup +_DllMainCRTStartup: + call *other(%rip) + call *__imp_foo(%rip) + ret diff --git a/lld/test/COFF/imports-static-lib.test b/lld/test/COFF/imports-static-lib.test index 63539cab9d860..8e9525dab5284 100644 --- a/lld/test/COFF/imports-static-lib.test +++ b/lld/test/COFF/imports-static-lib.test @@ -8,6 +8,7 @@ # RUN: not lld-link %t.foo.lib %t.main.obj -out:%t.dll -dll 2>&1 | FileCheck %s CHECK: error: undefined symbol: __declspec(dllimport) foo +CHECK: NOTE: a relevant symbol 'foo' is available in {{.*}}.foo.lib but cannot be used because it is not an import library. # Now do the same thing, but import the symbol from a import library. # RUN: llvm-mc --filetype=obj -triple=x86_64-windows-msvc %t.dir/foo_dll_main.s -o %t.foo_dll_main.obj From 8fed6a8e1a1ec0ff529a8b6730fdda1d04c4d5a7 Mon Sep 17 00:00:00 2001 From: Alexandre Ganea Date: Fri, 4 Apr 2025 17:58:32 -0400 Subject: [PATCH 3/3] Make expectation of the test case more explicit --- lld/test/COFF/imports-static-lib-indirect.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lld/test/COFF/imports-static-lib-indirect.test b/lld/test/COFF/imports-static-lib-indirect.test index 0ecb74a13de69..beda0d7a31afd 100644 --- a/lld/test/COFF/imports-static-lib-indirect.test +++ b/lld/test/COFF/imports-static-lib-indirect.test @@ -7,7 +7,7 @@ # RUN: llvm-lib %t.other.obj -out:%t.other.lib # RUN: lld-link %t.other.lib %t.main.obj -out:%t.dll -dll 2>&1 | FileCheck %s -CHECK: warning: {{.*}} [LNK4217] +CHECK: warning: {{.*}} locally defined symbol imported: foo {{.*}} [LNK4217] #--- other.s .text