[LLD][COFF] Make unresolved symbol search behavior compliant with MSVC link.exe #85290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aganea
requested review from
tru,
rnk,
compnerd,
mstorsjo,
MaskRay,
zmodem and
sylvain-audi
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-platform-windows Author: Alexandre Ganea (aganea) ChangesBefore this PR, unresolved symbols were always resolved depending on the order of inputs on the command line. The first input to provide a symbol would win. Subsequent inputs to provide the same symbol would either error out, or in case of archives (LIBs) would be ignored. The MSVC documentation however suggests a subtle difference for resolving unresolved symbols, once an archive OBJ is brought in by another symbol. As per: https://learn.microsoft.com/en-us/cpp/build/reference/link-input-files?view=msvc-170 > Object files on the command line are processed in the order they appear on the command line. Libraries are searched in command line order as well, with the following caveat: Symbols that are unresolved when bringing in an object file from a library are searched for in that library first, and then the following libraries from the command line and /DEFAULTLIB (Specify default library) directives, and then to any libraries at the beginning of the command line. This PR aligns the LLD behavior with MSVC. The approach I've taken is to keep (in the symbol table) all the duplicated lazy symbols exposed by input archives. More specifically, the first symbol for a given symbol name is pushed in the symbol table as usual, and any subsequent (lazy) symbols with the same name will be linked to that first symbol in a (intrusive) linked list. When an archive OBJ is pulled in through another symbol, any unresolved symbols for that OBJ are resolved by searching through the linked list, starting at the OBJ's specific archive position. For example: In this case, In practical terms, the unresolved symbol searching remains O(1) time, unless N archives are exposing the same symbol name, which makes the worst case O(N) time for that given symbol. This fixes a long-standing bug we were seeing from both Rust and C++, see: #82050 Patch is 26.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85290.diff 9 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 22ee2f133be98a..38e0392a876307 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -187,7 +187,8 @@ MemoryBufferRef LinkerDriver::takeBuffer(std::unique_ptr<MemoryBuffer> mb) {
}
void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
- bool wholeArchive, bool lazy) {
+ bool wholeArchive, bool lazy,
+ ArchiveFile *parent) {
StringRef filename = mb->getBufferIdentifier();
MemoryBufferRef mbref = takeBuffer(std::move(mb));
@@ -213,11 +214,11 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
ctx.symtab.addFile(make<ArchiveFile>(ctx, mbref));
break;
case file_magic::bitcode:
- ctx.symtab.addFile(make<BitcodeFile>(ctx, mbref, "", 0, lazy));
+ ctx.symtab.addFile(make<BitcodeFile>(ctx, mbref, "", 0, lazy, parent));
break;
case file_magic::coff_object:
case file_magic::coff_import_library:
- ctx.symtab.addFile(make<ObjFile>(ctx, mbref, lazy));
+ ctx.symtab.addFile(make<ObjFile>(ctx, mbref, lazy, parent));
break;
case file_magic::pdb:
ctx.symtab.addFile(make<PDBInputFile>(ctx, mbref));
@@ -242,7 +243,9 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
}
}
-void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy) {
+void LinkerDriver::enqueuePath(
+ StringRef path, bool wholeArchive, bool lazy,
+ std::optional<std::shared_future<ArchiveFile *>> parent) {
auto future = std::make_shared<std::future<MBErrPair>>(
createFutureForFile(std::string(path)));
std::string pathStr = std::string(path);
@@ -281,13 +284,15 @@ void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy) {
else
error(msg + "; did you mean '" + nearest + "'");
} else
- ctx.driver.addBuffer(std::move(mb), wholeArchive, lazy);
+ ctx.driver.addBuffer(std::move(mb), wholeArchive, lazy,
+ parent ? parent->get() : nullptr);
});
}
void LinkerDriver::addArchiveBuffer(MemoryBufferRef mb, StringRef symName,
StringRef parentName,
- uint64_t offsetInArchive) {
+ uint64_t offsetInArchive,
+ ArchiveFile *parent) {
file_magic magic = identify_magic(mb.getBuffer());
if (magic == file_magic::coff_import_library) {
InputFile *imp = make<ImportFile>(ctx, mb);
@@ -298,10 +303,10 @@ void LinkerDriver::addArchiveBuffer(MemoryBufferRef mb, StringRef symName,
InputFile *obj;
if (magic == file_magic::coff_object) {
- obj = make<ObjFile>(ctx, mb);
+ obj = make<ObjFile>(ctx, mb, /*lazy=*/false, parent);
} else if (magic == file_magic::bitcode) {
- obj =
- make<BitcodeFile>(ctx, mb, parentName, offsetInArchive, /*lazy=*/false);
+ obj = make<BitcodeFile>(ctx, mb, parentName, offsetInArchive,
+ /*lazy=*/false, parent);
} else if (magic == file_magic::coff_cl_gl_object) {
error(mb.getBufferIdentifier() +
": is not a native COFF file. Recompile without /GL?");
@@ -318,7 +323,8 @@ void LinkerDriver::addArchiveBuffer(MemoryBufferRef mb, StringRef symName,
void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
const Archive::Symbol &sym,
- StringRef parentName) {
+ StringRef parentName,
+ ArchiveFile *parent) {
auto reportBufferError = [=](Error &&e, StringRef childName) {
fatal("could not get the buffer for the member defining symbol " +
@@ -335,7 +341,7 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
enqueueTask([=]() {
llvm::TimeTraceScope timeScope("Archive: ", mb.getBufferIdentifier());
ctx.driver.addArchiveBuffer(mb, toCOFFString(ctx, sym), parentName,
- offsetInArchive);
+ offsetInArchive, parent);
});
return;
}
@@ -356,7 +362,15 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
// used as the buffer identifier.
ctx.driver.addArchiveBuffer(takeBuffer(std::move(mbOrErr.first)),
toCOFFString(ctx, sym), "",
- /*OffsetInArchive=*/0);
+ /*OffsetInArchive=*/0, parent);
+ });
+}
+
+void LinkerDriver::enqueueLazyFile(InputFile *file) {
+ enqueueTask([=]() {
+ // Once it has been enqued, it cannot be lazy anymore.
+ file->lazy = false;
+ ctx.symtab.addFile(file);
});
}
@@ -2111,17 +2125,30 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
{
llvm::TimeTraceScope timeScope2("Parse & queue inputs");
bool inLib = false;
+ std::optional<std::shared_future<ArchiveFile *>> inLibArchive;
for (auto *arg : args) {
switch (arg->getOption().getID()) {
case OPT_end_lib:
if (!inLib)
error("stray " + arg->getSpelling());
inLib = false;
+ inLibArchive = std::nullopt;
break;
case OPT_start_lib:
if (inLib)
error("nested " + arg->getSpelling());
inLib = true;
+ // In is important to create a fake archive here so that we remember its
+ // placement on the command-line. This will be later needed to resolve
+ // symbols in the archive order required by the MSVC specification.
+ {
+ auto a = std::make_shared<std::promise<ArchiveFile *>>();
+ inLibArchive = a->get_future().share();
+ enqueueTask([=] {
+ a->set_value(
+ make<ArchiveFile>(ctx, MemoryBufferRef({}, "<cmdline-lib>")));
+ });
+ }
break;
case OPT_wholearchive_file:
if (std::optional<StringRef> path = findFileIfNew(arg->getValue()))
@@ -2129,7 +2156,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
break;
case OPT_INPUT:
if (std::optional<StringRef> path = findFileIfNew(arg->getValue()))
- enqueuePath(*path, isWholeArchive(*path), inLib);
+ enqueuePath(*path, isWholeArchive(*path), inLib, inLibArchive);
break;
default:
// Ignore other options.
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index fa54de05befb58..da3c41e1bca734 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -22,6 +22,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/TarWriter.h"
#include "llvm/WindowsDriver/MSVCPaths.h"
+#include <future>
#include <memory>
#include <optional>
#include <set>
@@ -91,13 +92,20 @@ class LinkerDriver {
// Used by ArchiveFile to enqueue members.
void enqueueArchiveMember(const Archive::Child &c, const Archive::Symbol &sym,
- StringRef parentName);
+ StringRef parentName,
+ ArchiveFile *parent = nullptr);
- void enqueuePDB(StringRef Path) { enqueuePath(Path, false, false); }
+ void enqueuePDB(StringRef Path) {
+ enqueuePath(Path, false, false, /*parent=*/std::nullopt);
+ }
MemoryBufferRef takeBuffer(std::unique_ptr<MemoryBuffer> mb);
- void enqueuePath(StringRef path, bool wholeArchive, bool lazy);
+ void enqueuePath(
+ StringRef path, bool wholeArchive, bool lazy,
+ std::optional<std::shared_future<ArchiveFile *>> parent = std::nullopt);
+
+ void enqueueLazyFile(InputFile *file);
std::unique_ptr<llvm::TarWriter> tar; // for /linkrepro
@@ -182,10 +190,11 @@ class LinkerDriver {
StringRef findDefaultEntry();
WindowsSubsystem inferSubsystem();
- void addBuffer(std::unique_ptr<MemoryBuffer> mb, bool wholeArchive,
- bool lazy);
+ void addBuffer(std::unique_ptr<MemoryBuffer> mb, bool wholeArchive, bool lazy,
+ ArchiveFile *parent = nullptr);
void addArchiveBuffer(MemoryBufferRef mbref, StringRef symName,
- StringRef parentName, uint64_t offsetInArchive);
+ StringRef parentName, uint64_t offsetInArchive,
+ ArchiveFile *parent = nullptr);
void enqueueTask(std::function<void()> task);
bool run();
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 037fae45242c6f..42cdd1cf3b6c2c 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -94,9 +94,12 @@ static bool ignoredSymbolName(StringRef name) {
}
ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
- : InputFile(ctx, ArchiveKind, m) {}
+ : InputFile(ctx, ArchiveKind, m, /*lazy=*/true) {
+ static unsigned Order = 0;
+ CmdLineIndex = Order++;
+}
-void ArchiveFile::parse() {
+void ArchiveFile::parseLazy() {
// Parse a MemoryBufferRef as an archive file.
file = CHECK(Archive::create(mb), this);
@@ -115,7 +118,7 @@ void ArchiveFile::addMember(const Archive::Symbol &sym) {
if (!seen.insert(c.getChildOffset()).second)
return;
- ctx.driver.enqueueArchiveMember(c, sym, getName());
+ ctx.driver.enqueueArchiveMember(c, sym, getName(), this);
}
std::vector<MemoryBufferRef> lld::coff::getArchiveMembers(Archive *file) {
@@ -1000,8 +1003,8 @@ void ImportFile::parse() {
BitcodeFile::BitcodeFile(COFFLinkerContext &ctx, MemoryBufferRef mb,
StringRef archiveName, uint64_t offsetInArchive,
- bool lazy)
- : InputFile(ctx, BitcodeKind, mb, lazy) {
+ bool lazy, ArchiveFile *parent)
+ : InputFile(ctx, BitcodeKind, mb, lazy), parent(parent) {
std::string path = mb.getBufferIdentifier().str();
if (ctx.config.thinLTOIndexOnly)
path = replaceThinLTOSuffix(mb.getBufferIdentifier(),
diff --git a/lld/COFF/InputFiles.h b/lld/COFF/InputFiles.h
index 3b55cd791bfda2..7070f51fdf78ac 100644
--- a/lld/COFF/InputFiles.h
+++ b/lld/COFF/InputFiles.h
@@ -66,7 +66,6 @@ class InputFile {
enum Kind {
ArchiveKind,
ObjectKind,
- LazyObjectKind,
PDBKind,
ImportKind,
BitcodeKind,
@@ -105,7 +104,7 @@ class InputFile {
public:
// True if this is a lazy ObjFile or BitcodeFile.
- bool lazy = false;
+ bool lazy;
};
// .lib or .a file.
@@ -113,23 +112,30 @@ class ArchiveFile : public InputFile {
public:
explicit ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m);
static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
- void parse() override;
+ void parse() override{};
+ void parseLazy();
// Enqueues an archive member load for the given symbol. If we've already
// enqueued a load for the same archive member, this function does nothing,
// which ensures that we don't load the same member more than once.
void addMember(const Archive::Symbol &sym);
-private:
std::unique_ptr<Archive> file;
+
+ // The order this archive was seen on the cmd-line. This is later needed for
+ // resolving undefined symbols in archive OBJs.
+ uint32_t CmdLineIndex;
+
+private:
llvm::DenseSet<uint64_t> seen;
};
// .obj or .o file. This may be a member of an archive file.
class ObjFile : public InputFile {
public:
- explicit ObjFile(COFFLinkerContext &ctx, MemoryBufferRef m, bool lazy = false)
- : InputFile(ctx, ObjectKind, m, lazy) {}
+ explicit ObjFile(COFFLinkerContext &ctx, MemoryBufferRef m, bool lazy = false,
+ ArchiveFile *parent = nullptr)
+ : InputFile(ctx, ObjectKind, m, lazy), parent(parent) {}
static bool classof(const InputFile *f) { return f->kind() == ObjectKind; }
void parse() override;
void parseLazy();
@@ -182,6 +188,9 @@ class ObjFile : public InputFile {
// True if this file was compiled with /guard:ehcont.
bool hasGuardEHCont() { return feat00Flags & 0x4000; }
+ // Whether this Obj buffer is part of an archive.
+ ArchiveFile *parent;
+
// Pointer to the PDB module descriptor builder. Various debug info records
// will reference object files by "module index", which is here. Things like
// source files and section contributions are also recorded here. Will be null
@@ -369,7 +378,7 @@ class BitcodeFile : public InputFile {
public:
explicit BitcodeFile(COFFLinkerContext &ctx, MemoryBufferRef mb,
StringRef archiveName, uint64_t offsetInArchive,
- bool lazy);
+ bool lazy = false, ArchiveFile *parent = nullptr);
~BitcodeFile();
static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
ArrayRef<Symbol *> getSymbols() { return symbols; }
@@ -377,6 +386,9 @@ class BitcodeFile : public InputFile {
void parseLazy();
std::unique_ptr<llvm::lto::InputFile> obj;
+ // Whether this bitcode buffer is part of an archive.
+ ArchiveFile *parent;
+
private:
void parse() override;
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 44aa506d2c35da..f570e8c211f43d 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -54,8 +54,10 @@ void SymbolTable::addFile(InputFile *file) {
if (file->lazy) {
if (auto *f = dyn_cast<BitcodeFile>(file))
f->parseLazy();
- else
- cast<ObjFile>(file)->parseLazy();
+ else if (auto *o = dyn_cast<ObjFile>(file))
+ o->parseLazy();
+ else if (auto *a = dyn_cast<ArchiveFile>(file))
+ a->parseLazy();
} else {
file->parse();
if (auto *f = dyn_cast<ObjFile>(file)) {
@@ -102,7 +104,7 @@ static void forceLazy(Symbol *s) {
}
case Symbol::Kind::LazyObjectKind: {
InputFile *file = cast<LazyObject>(s)->file;
- file->ctx.symtab.addFile(file);
+ file->ctx.driver.enqueueLazyFile(file);
break;
}
case Symbol::Kind::LazyDLLSymbolKind: {
@@ -562,6 +564,57 @@ std::pair<Symbol *, bool> SymbolTable::insert(StringRef name, InputFile *file) {
return result;
}
+static LazyIntrusiveNode *lazyNode(Symbol *s) {
+ if (auto *sym = dyn_cast<LazyArchive>(s))
+ return &sym->node;
+ if (auto *sym = dyn_cast<LazyObject>(s))
+ return &sym->node;
+ return nullptr;
+}
+
+static ArchiveFile *lazyParent(InputFile *f) {
+ if (!f)
+ return nullptr;
+ if (auto *obj = dyn_cast<ObjFile>(f))
+ return obj->parent;
+ if (auto *obj = dyn_cast<BitcodeFile>(f))
+ return obj->parent;
+ return nullptr;
+}
+
+static ArchiveFile *lazyArchive(Symbol *s) {
+ if (auto *sym = dyn_cast<LazyArchive>(s))
+ return sym->file;
+ if (auto *sym = dyn_cast<LazyObject>(s))
+ return lazyParent(sym->file);
+ return nullptr;
+}
+
+// The search behavior for undefined symbols is different when the OBJ
+// was pulled from an archive (LIB). This is documented here:
+// https://learn.microsoft.com/en-us/cpp/build/reference/link-input-files?view=msvc-170
+// "Object files on the command line are processed in the order they
+// appear on the command line. Libraries are searched in command line
+// order as well, with the following caveat: Symbols that are unresolved
+// when bringing in an object file from a library are searched for in
+// that library first, and then the following libraries from the command
+// line and /DEFAULTLIB (Specify default library) directives, and then
+// to any libraries at the beginning of the command line."
+static Symbol *searchArchiveSymbol(Symbol *s, ArchiveFile *pivot) {
+ auto &Alloc = getSpecificAllocSingleton<SymbolUnion>().Allocator;
+ Symbol *curr = s;
+ for (;;) {
+ if (lazyArchive(curr)->CmdLineIndex >= pivot->CmdLineIndex)
+ return curr;
+ uint32_t next = lazyNode(curr)->next;
+ if (!next)
+ break;
+ curr = reinterpret_cast<LazyArchive *>(
+ Alloc.fromAlignedIndex<SymbolUnion>(next));
+ }
+ return s;
+}
+
Symbol *SymbolTable::addUndefined(StringRef name, InputFile *f,
bool isWeakAlias) {
auto [s, wasInserted] = insert(name, f);
@@ -569,11 +622,43 @@ Symbol *SymbolTable::addUndefined(StringRef name, InputFile *f,
replaceSymbol<Undefined>(s, name);
return s;
}
- if (s->isLazy())
+ if (s->isLazy()) {
+ if (ArchiveFile *parent = lazyParent(f)) {
+ Symbol *selected = searchArchiveSymbol(s, parent);
+ forceLazy(selected);
+ // Now that we have selected a symbol, we don't need the linked list of
+ // `LazyArchive`s anymore. Collapse to the selected symbol.
+ memcpy(s, selected, sizeof(SymbolUnion));
+ return s;
+ }
forceLazy(s);
+ }
return s;
}
+// This creates a linked list of archives where a specific symbol was seen.
+// We later walk that list if a undefined symbol needs to be resolved from an
+// archive OBJ.
+template <typename T, typename... ArgT>
+static void chainLazy(LazyIntrusiveNode *front, ArgT &&...arg) {
+ // Chain with symbols defined in other archives
+ Symbol *newSym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
+ newSym->canInline = true;
+ replaceSymbol<T>(newSym, std::forward<ArgT>(arg)...);
+
+ auto &Alloc = getSpecificAllocSingleton<SymbolUnion>().Allocator;
+ uint32_t index = Alloc.identifyKnownAlignedObject<SymbolUnion>(newSym);
+
+ if (!front->next)
+ front->next = index;
+ if (front->last) {
+ Symbol *last = reinterpret_cast<Symbol *>(
+ Alloc.fromAlignedIndex<SymbolUnion>(front->last));
+ lazyNode(last)->next = index;
+ }
+ front->last = index;
+}
+
void SymbolTable::addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym) {
StringRef name = sym.getName();
auto [s, wasInserted] = insert(name);
@@ -581,6 +666,10 @@ void SymbolTable::addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym) {
replaceSymbol<LazyArchive>(s, f, sym);
return;
}
+ if (auto *n = lazyNode(s)) {
+ chainLazy<LazyArchive>(n, f, sym);
+ return;
+ }
auto *u = dyn_cast<Undefined>(s);
if (!u || u->weakAlias || s->pendingArchiveLoad)
return;
@@ -588,19 +677,22 @@ void SymbolTable::addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym) {
f->addMember(sym);
}
-void SymbolTable::addLazyObject(InputFile *f, StringRef n) {
+void SymbolTable::addLazyObject(InputFile *f, StringRef name) {
assert(f->lazy);
- auto [s, wasInserted] = insert(n, f);
+ auto [s, wasInserted] = insert(name, f);
if (wasInserted) {
- replaceSymbol<LazyObject>(s, f, n);
+ replaceSymbol<LazyObject>(s, f, name);
+ return;
+ }
+ if (auto *n = lazyNode(s)) {
+ chainLazy<LazyObject>(n, f, name);
return;
}
auto *u = dyn_cast<Undefined>(s);
if (!u || u->weakAlias || s->pendingArchiveLoad)
return;
s->pendingArchiveLoad = true;
- f->lazy = false;
- addFile(f);
+ f->ctx.driver.enqueueLazyFile(f);
}
void SymbolTable::addLazyDLLSymbol(DLLFile *f, DLLFile::Symbol *sym,
diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
index ca69fb2d052706..1577406c8626cf 100644
--- a/lld/COFF/Symbols.h
+++ b/lld/COFF/Symbols.h
@@ -286,6 +286,15 @@ class DefinedSynthetic : public Defined {
uint32_t offset;
};
+// Keep track of symbols with the same name exposed by archives. This is
+// required to later resolve unresolved symbols in the same order as required
+// by the MSVC spec. These are indexes in the specific bump allocator for
+// SymbolUnion.
+struct LazyIntrusiveNode {
+ uint32_t next = 0;
+ uint32_t last = 0;
+};
+
// This class represents a symbol defined in an archive file. It is
// created from an archive file header, and it knows how to load an
// object file from an archive to replace itself with a defined
@@ -302,6 +311,7 @@ class LazyArchive : public Symbol {
ArchiveFile *file;
const Archive::Symbol sym;
+ LazyIntrusiveNode node;
};
class LazyObject : public Symbol {
@@ -309,6 +319,7 @@ class LazyObject : public Symbol {
LazyObject(InputFile *f, StringRef n) : Symbol(LazyObjectKind, n), file(f) {}
static bool classof(const Symbol *s) { return s->kind() == LazyObjectKind; }
InputFile *file;
+ LazyIntrusiveNode node;
};
// MinGW only.
diff --git a/lld/test/COFF/duplicate-imp-func.s b/lld/test/COFF/duplicate-imp-func.s
index fc0cf1ef6ae051..631c714c951f77 100644
--- a/lld/test/COFF/duplicate-imp-func.s
+++ b/lld/test/COFF/duplicate-imp-func.s
@@ -28,8 +28,10 @@
# Once the import library member from %t.lib.dll.a gets loaded, libfunc
# and __imp_libfunc already are defined.
-# Just check that this fails...
[truncated]
|
aganea
commented
Mar 14, 2024
| // placement on the command-line. This will be later needed to resolve | ||
| // symbols in the archive order required by the MSVC specification. | ||
| { | ||
| auto a = std::make_shared<std::promise<ArchiveFile *>>(); |
There was a problem hiding this comment.
Review note: this is made a shared_ptr<promise> simply because lambdas cannot capture non-CopyConstructible object instances, because of https://timsong-cpp.github.io/cppwp/n4659/func.wrap.func.con#7
aganea
changed the title
directive section
aganea
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This is ready for review. This has been tested with a two-stage LLVM toolchain, then on a Unreal Engine-based project. I've also added support for handling |
|
Gentle ping! Is anyone able to review this please? I can jump on a call if a more interactive review is desired. |
|
This is something I'd like @rnk to have a look at. |
|
Ping. |
|
@rnk - do you have time to have a look at this one? |
|
Ping again, we also really want this fix in order to unblock our progress to support ld-lld for linking. Thanks |
|
Ping again, we also need this to support using lld-link for building Unreal Engine 5 with ClangCL toolchain |
|
fyi, I binary patched the checked in xinput import lib in the unreal engine depot and removed the export of winmain.. this is a workaround until we get this fix submitted. |
cjacek
reviewed
Oct 24, 2024
Comment on lines
+511
to
+515
|
|
||
| // If we are running off the low-priority task list, execute and drain the | ||
| // high priority task list before going any further. This is to ensure symbols | ||
| // provided by /DEFAULTLIB archives are linked properly in the symbol table. | ||
| executeFirstQueue(firstTaskQueue); |
There was a problem hiding this comment.
I’m wondering if we could simplify this by having run() handle it directly without the need for additional assistance. For example, the attached patch also passes the tests.
| ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m) | ||
| : InputFile(ctx, ArchiveKind, m) {} | ||
| : InputFile(ctx, ArchiveKind, m, /*lazy=*/true) { | ||
| static unsigned Order = 0; |
There was a problem hiding this comment.
It could be a member of COFFLinkerContext.
Comment on lines
+293
to
+296
| struct LazyIntrusiveNode { | ||
| uint32_t next = 0; | ||
| uint32_t last = 0; | ||
| }; |
There was a problem hiding this comment.
I assume it uses indices instead of pointers to avoid enlarging SymbolUnion. If that’s the case, perhaps we could avoid storing last. To maintain O(1) insertion, we could always insert the element just after the front element. The order would then be almost reversed: (first), (last), (second to last), (third to last), and so on. Lookup would need to compensate by returning either the front or the last matching element. With the saved space, we could store next as a pointer and eliminate the need for fromAlignedIndex.
rnk
reviewed
Oct 29, 2024
There was a problem hiding this comment.
I haven't finished reviewing this, but I wanted to say I read the problem statement and the reproducer and I think I understand the failure mode from the Rust issue where some unrelated exported function from an import library was preferred over the symbol defined local to a static library.
I think my main consideration is, how will this behavior interact with any future attempt to parallelize symbol resolution? I'm still dreaming of this, but maybe everyone else has moved on. The critical requirement is that we can define some kind of global symbol priority comparator, and the need to "prefer" library local symbols seems like it would complicate that.
Anway, I'll have to revisit this tomorrow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR, unresolved symbols were always resolved depending on the order of inputs on the command line. The first input (.LIB or .OBJ) to provide a symbol would win. Subsequent inputs to provide the same symbol would either error out, or in case of archives (LIBs) would be ignored.
The MSVC documentation however suggests a subtle difference for resolving unresolved symbols, once an archive OBJ is brought in by another symbol. As per: https://learn.microsoft.com/en-us/cpp/build/reference/link-input-files?view=msvc-170
(Thanks to Grant Richins from the MSVC linker team who pointed me to that paragraph)
This PR aligns the LLD behavior with MSVC. The approach I've taken is to keep (in the symbol table) all the duplicated lazy symbols exposed by input archives. More specifically, the first symbol for a given symbol name is pushed in the symbol table as usual, and any subsequent (lazy) symbols with the same name will be linked to that first symbol in a (intrusive) linked list. When an archive OBJ is pulled in through another symbol, any unresolved symbols for that OBJ are resolved by searching through the linked list, starting at the OBJ's specific archive position. For example:
In this case,
main.objpulls on a symbol insideb.lib, which pulls onb1.objin that archive.b1.objthen requires another unresolved symbolf, and starts searching atb.lib, thenc.lib, thena.lib. Previously, we always pulled the symbol from the first archive exposing it,a.libin this case.In practical terms, the unresolved symbol searching remains O(1) time, unless N archives are exposing the same symbol name, which makes the worst case O(N) time for that given symbol.
This fixes a long-standing bug we were seeing from both Rust and C++, see: #82050
Additional note: I also had to fix the behavior of command-line archives introduced in 7dc5e7a, to be symmetric with regular LIB files. Therefore OBJs enclosed in
-start-lib/-end-libare now lazily read, to postpone resolving of their symbols to after the entire command-line was processed.