Skip to content

[LLD][COFF] Make unresolved symbol search behavior compliant with MSVC link.exe #85290

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.

Sign up for GitHub

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

[LLD][COFF] Make unresolved symbol search behavior compliant with MSVC link.exe #85290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d5c4adf
[LLD][COFF] Align unresolved symbols search behavior with MSVC link.exe
aganea Mar 14, 2024
b1149e1
Revert unneeded changes
aganea Mar 14, 2024
6c2a777
Test cmd-line libraries
aganea Mar 14, 2024
ad59e48
LLD release notes
aganea Mar 15, 2024
b115c60
Add another egde case where symbols could be pulled/resolved from two…
aganea Mar 15, 2024
9d910c7
Missing assert for cmd-line archives
aganea Mar 15, 2024
865dc26
Support the new behavior with /DEFAULTLIB on the cmd-line or in
aganea Mar 18, 2024
114870b
Fix chaining with pending load symbols
aganea Mar 19, 2024
0ece320
Add /DEFAULTLIB tests
aganea Apr 8, 2024
4d6fcd8
Review fixes
aganea Apr 8, 2024
87a90c3
More review fixes
aganea Apr 8, 2024
c2e7c78
clang-format
aganea Apr 8, 2024
0f5d47e
Typo
aganea Apr 8, 2024
baeca96
Fix lazy node casting in the presence of wrapped symbols (MinGW -wrap)
aganea Jun 17, 2024
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
106 changes: 81 additions & 25 deletions lld/COFF/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "llvm/Support/Parallel.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/Support/TarWriter.h"
#include "llvm/Support/TargetSelect.h"
#include "llvm/Support/VirtualFileSystem.h"
Expand Down Expand Up @@ -187,7 +188,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));
Expand All @@ -213,11 +215,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));
Expand All @@ -242,7 +244,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);
Expand Down Expand Up @@ -281,13 +285,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);
Expand All @@ -298,10 +304,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?");
Expand All @@ -318,7 +324,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 " +
Expand All @@ -332,10 +339,10 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
if (!mbOrErr)
reportBufferError(mbOrErr.takeError(), check(c.getFullName()));
MemoryBufferRef mb = mbOrErr.get();
enqueueTask([=]() {
enqueueSecondaryTask([=]() {
llvm::TimeTraceScope timeScope("Archive: ", mb.getBufferIdentifier());
ctx.driver.addArchiveBuffer(mb, toCOFFString(ctx, sym), parentName,
offsetInArchive);
offsetInArchive, parent);
});
return;
}
Expand All @@ -346,7 +353,7 @@ void LinkerDriver::enqueueArchiveMember(const Archive::Child &c,
toCOFFString(ctx, sym));
auto future =
std::make_shared<std::future<MBErrPair>>(createFutureForFile(childName));
enqueueTask([=]() {
enqueueSecondaryTask([=]() {
auto mbOrErr = future->get();
if (mbOrErr.second)
reportBufferError(errorCodeToError(mbOrErr.second), childName);
Expand All @@ -356,7 +363,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) {
enqueueSecondaryTask([=]() {
// Once it has been enqueued, it cannot be lazy anymore.
file->lazy = false;
ctx.symtab.addFile(file);
});
}

Expand All @@ -365,6 +380,24 @@ bool LinkerDriver::isDecorated(StringRef sym) {
(!ctx.config.mingw && sym.contains('@'));
}

static LLVM_THREAD_LOCAL bool executingFirstQueue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than TLS for every thread, can this be a LinkerDriver field and these helpers private methods?


static bool executeQueue(std::list<std::function<void()>> &queue) {
bool didWork = !queue.empty();
while (!queue.empty()) {
queue.front()();
queue.pop_front();
}
return didWork;
}

static bool executeFirstQueue(std::list<std::function<void()>> &queue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments here about the purpose of the first queue (to load only .objs from the command line) would be helpful.

if (executingFirstQueue)
return false;
SaveAndRestore s(executingFirstQueue, true);
return executeQueue(queue);
}

// Parses .drectve section contents and returns a list of files
// specified by /defaultlib.
void LinkerDriver::parseDirectives(InputFile *file) {
Expand Down Expand Up @@ -475,6 +508,11 @@ void LinkerDriver::parseDirectives(InputFile *file) {
toString(file) + ")");
}
}

// 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);
Comment on lines +511 to +515
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

// Find file from search paths. You can omit ".obj", this function takes
Expand Down Expand Up @@ -1057,18 +1095,19 @@ void LinkerDriver::parseModuleDefs(StringRef path) {
}

void LinkerDriver::enqueueTask(std::function<void()> task) {
taskQueue.push_back(std::move(task));
firstTaskQueue.push_back(std::move(task));
}

void LinkerDriver::enqueueSecondaryTask(std::function<void()> task) {
secondaryTaskQueue.push_back(std::move(task));
}

bool LinkerDriver::run() {
llvm::TimeTraceScope timeScope("Read input files");
ScopedTimer t(ctx.inputFileTimer);

bool didWork = !taskQueue.empty();
while (!taskQueue.empty()) {
taskQueue.front()();
taskQueue.pop_front();
}
bool didWork = executeFirstQueue(firstTaskQueue);
didWork |= executeQueue(secondaryTaskQueue);
return didWork;
}

Expand Down Expand Up @@ -2111,25 +2150,38 @@ 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 *>>();
Copy link
Member Author

@aganea aganea Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is nitty code golf, but can this be implemented with an explicit lambda capture and std::move? This SO pattern is what I was thinking of.

Maybe the task gets copied around more, so if not, then the shared_ptr is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did try several things, including what you're suggesting, as well as the std::bind trick, but nothing else works, besides the share_ptr above.

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()))
enqueuePath(*path, true, inLib);
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.
Expand All @@ -2138,8 +2190,10 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
}
}

// Read all input files given via the command line.
run();
// Read all input files given via the command line. Do not process the
// dependent OBJs pulled from archives just yet, since we need to push the
// default libs first.
executeFirstQueue(firstTaskQueue);
if (errorCount())
return;

Expand Down Expand Up @@ -2443,9 +2497,11 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {

if (args.hasArg(OPT_include_optional)) {
// Handle /includeoptional
for (auto *arg : args.filtered(OPT_include_optional))
if (isa_and_nonnull<LazyArchive>(ctx.symtab.find(arg->getValue())))
for (auto *arg : args.filtered(OPT_include_optional)) {
Symbol *sym = ctx.symtab.find(arg->getValue());
if (sym && (isa<LazyArchive>(sym) || isa<LazyObject>(sym)))
addUndefined(arg->getValue());
}
}
} while (run());
}
Expand Down
27 changes: 21 additions & 6 deletions lld/COFF/Driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -91,13 +92,18 @@ 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); }

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

Expand Down Expand Up @@ -182,15 +188,24 @@ 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);
void enqueueSecondaryTask(std::function<void()> task);
bool run();

std::list<std::function<void()>> taskQueue;
// The first queue contains all direct command-line inputs, all /defaultlib
// LIBs, provided on the command-line or in a directives section. The second
// queue is meant for lower-priority dependent OBJs pulled by a symbol from an
// archive. If there are items in both queues, the first one must be fully
// executed first before the second queue. This is important to ensure we pull
// on archives symbols in the order specified by the MSVC spec.
std::list<std::function<void()>> firstTaskQueue;
std::list<std::function<void()>> secondaryTaskQueue;
std::vector<StringRef> filePaths;
std::vector<MemoryBufferRef> resources;

Expand Down
Loading