[LLD][COFF] Don't dllimport from static libraries #134443

aganea · 2025-04-04T20:20:43Z

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 #131807

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 llvm#131807

aganea · 2025-04-04T20:21:12Z

+@glandium

llvmbot · 2025-04-04T20:21:15Z

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld

Author: Alexandre Ganea (aganea)

Changes

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 #131807

Full diff: https://github.com/llvm/llvm-project/pull/134443.diff

5 Files Affected:

(modified) lld/COFF/Driver.cpp (+2-4)
(modified) lld/COFF/SymbolTable.cpp (+12-8)
(modified) lld/COFF/SymbolTable.h (+1-4)
(added) lld/test/COFF/imports-static-lib.test (+32)
(removed) lld/test/COFF/undefined_lazy.test (-26)

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<const char *> 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<Symbol *, 8> undefs;
   DenseMap<Symbol *, Symbol *> 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<Defined>(imp)) {
         auto *d = cast<Defined>(imp);
         replaceSymbol<DefinedLocalImport>(sym, ctx, name, d);
@@ -513,7 +518,6 @@ bool SymbolTable::resolveRemainingUndefines() {
   reportProblemSymbols(
       undefs, ctx.config.warnLocallyDefinedImported ? &localImports : nullptr,
       false);
-  return foundLazy;
 }
 
 std::pair<Symbol *, bool> 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

llvmbot · 2025-04-04T20:21:16Z

@llvm/pr-subscribers-lld-coff

Author: Alexandre Ganea (aganea)

Changes

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 #131807

Full diff: https://github.com/llvm/llvm-project/pull/134443.diff

5 Files Affected:

(modified) lld/COFF/Driver.cpp (+2-4)
(modified) lld/COFF/SymbolTable.cpp (+12-8)
(modified) lld/COFF/SymbolTable.h (+1-4)
(added) lld/test/COFF/imports-static-lib.test (+32)
(removed) lld/test/COFF/undefined_lazy.test (-26)

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<const char *> 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<Symbol *, 8> undefs;
   DenseMap<Symbol *, Symbol *> 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<Defined>(imp)) {
         auto *d = cast<Defined>(imp);
         replaceSymbol<DefinedLocalImport>(sym, ctx, name, d);
@@ -513,7 +518,6 @@ bool SymbolTable::resolveRemainingUndefines() {
   reportProblemSymbols(
       undefs, ctx.config.warnLocallyDefinedImported ? &localImports : nullptr,
       false);
-  return foundLazy;
 }
 
 std::pair<Symbol *, bool> 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

mstorsjo

I think this looks reasonable, thanks!

IIRC #109082 wasn't for a concrete issue after all (the case where we found this was messed up in other ways anywya), but I'd wait for @glandium to confirm that too.

mstorsjo · 2025-04-04T20:41:56Z

lld/COFF/SymbolTable.cpp

+    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.";


Nit: an import library

Initially I felt confused by this message, but I think it may be the best way to phrase it. The other angle at this is, that if this wasn't a static library but object files, or members in a static library that we've already pulled in, then we'd actually link it successfully. But it's hard to suggest that in the diagnostic, and we probably don't want to push users that way anyway. So I guess this wording is fine.

I don't see this diagnostic tested though - I think it would be good to check that we do hit it.

Please see updated version. I've added another testcase to also validate your suggestion in #131807 (comment) -- which happens to only generate a warning.

mstorsjo

Looking good from my point of view now, thanks! But please wait for @glandium to reconfirm before landing. (I'm holding off of the approve-button until then.)

glandium · 2025-04-06T20:32:00Z

I /think/ this is fine. I'll look out for the fallout after it lands.

mstorsjo · 2025-04-08T20:41:22Z

As this is a crash fix for a regression in 20.x, we should probably consider backporting this to 20.x (especially as this doesn't seem to be the only case that hits this bug, see #134843), but we probably also first should give it a couple of days so that @glandium may pick it up and do some building with it.

aganea · 2025-04-15T21:39:05Z

/cherry-pick c75eac7

llvmbot · 2025-04-15T21:44:52Z

/cherry-pick c75eac7

Error: Command failed due to missing milestone.

aganea · 2025-04-15T21:48:41Z

/cherry-pick c75eac7

llvmbot · 2025-04-15T21:54:27Z

Failed to cherry-pick: c75eac7

https://github.com/llvm/llvm-project/actions/runs/14480041593

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

tstellar · 2025-04-15T21:56:16Z

@aganea Were you able to manually backport this?

aganea · 2025-05-02T21:57:29Z

Ah I see, there's an ambiguity:

C:\git\llvm-project>git cherry-pick c75eac7
error: short object ID c75eac7 is ambiguous
hint: The candidates are:
hint:   c75eac7c0347 commit 2025-04-07 - [LLD][COFF] Don't dllimport from static libraries (#134443)
hint:   c75eac723654 tree
fatal: bad revision 'c75eac7'

I'll try again with the longer ID.

aganea · 2025-05-02T21:57:44Z

/cherry-pick c75eac7

aganea · 2025-05-02T22:28:48Z

Don't seem to be working. I'll send a PR.

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. A hint is now displayed in stdout, mentioning that we should rather dllimport the symbol from a import library. Fixes llvm#131807

aganea requested review from mstorsjo, MaskRay and cjacek April 4, 2025 20:20

llvmbot added lld lld:COFF platform:windows labels Apr 4, 2025

mstorsjo reviewed Apr 4, 2025

View reviewed changes

aganea added 2 commits April 4, 2025 17:54

Simplify & add a test case

593bfe8

Make expectation of the test case more explicit

8fed6a8

mstorsjo reviewed Apr 6, 2025

View reviewed changes

mstorsjo approved these changes Apr 7, 2025

View reviewed changes

aganea merged commit c75eac7 into llvm:main Apr 7, 2025
11 checks passed

mstorsjo mentioned this pull request Apr 8, 2025

ld.lld crashing when linking GraphicsMagick #134843

Closed

aganea deleted the fix_lld_imports_from_static_lib branch April 8, 2025 23:58

aganea added this to the LLVM 20.X Release milestone Apr 15, 2025

github-project-automation bot added this to LLVM Release Status Apr 15, 2025

github-project-automation bot moved this to Needs Triage in LLVM Release Status Apr 15, 2025

llvmbot added the release:cherry-pick-failed label Apr 15, 2025

tstellar moved this from Needs Triage to Needs Pull Request in LLVM Release Status Apr 15, 2025

aganea mentioned this pull request May 2, 2025

[LLD][COFF] Don't dllimport from static libraries (#134443) #138354

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLD][COFF] Don't dllimport from static libraries #134443

[LLD][COFF] Don't dllimport from static libraries #134443

aganea commented Apr 4, 2025

aganea commented Apr 4, 2025

llvmbot commented Apr 4, 2025 •

edited

Loading

llvmbot commented Apr 4, 2025

mstorsjo left a comment

mstorsjo Apr 4, 2025

aganea Apr 4, 2025 •

edited

Loading

mstorsjo left a comment

glandium commented Apr 6, 2025

mstorsjo commented Apr 8, 2025

aganea commented Apr 15, 2025

llvmbot commented Apr 15, 2025

aganea commented Apr 15, 2025

llvmbot commented Apr 15, 2025

tstellar commented Apr 15, 2025

aganea commented May 2, 2025

aganea commented May 2, 2025

aganea commented May 2, 2025

[LLD][COFF] Don't dllimport from static libraries #134443

[LLD][COFF] Don't dllimport from static libraries #134443

Conversation

aganea commented Apr 4, 2025

aganea commented Apr 4, 2025

llvmbot commented Apr 4, 2025 • edited Loading

llvmbot commented Apr 4, 2025

mstorsjo left a comment

Choose a reason for hiding this comment

mstorsjo Apr 4, 2025

Choose a reason for hiding this comment

aganea Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

mstorsjo left a comment

Choose a reason for hiding this comment

glandium commented Apr 6, 2025

mstorsjo commented Apr 8, 2025

aganea commented Apr 15, 2025

llvmbot commented Apr 15, 2025

aganea commented Apr 15, 2025

llvmbot commented Apr 15, 2025

tstellar commented Apr 15, 2025

aganea commented May 2, 2025

aganea commented May 2, 2025

aganea commented May 2, 2025

llvmbot commented Apr 4, 2025 •

edited

Loading

aganea Apr 4, 2025 •

edited

Loading