Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions lld/COFF/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
20 changes: 12 additions & 8 deletions lld/COFF/SymbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@aganea aganea Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}

void SymbolTable::loadMinGWSymbols() {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 1 addition & 4 deletions lld/COFF/SymbolTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
32 changes: 32 additions & 0 deletions lld/test/COFF/imports-static-lib.test
Original file line number Diff line number Diff line change
@@ -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
26 changes: 0 additions & 26 deletions lld/test/COFF/undefined_lazy.test

This file was deleted.