Skip to content

[lld][COFF] Add /linkreprofullpathrsp flag #165449

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 3 commits into
base: main
Choose a base branch
from
+79 −8
Open

[lld][COFF] Add /linkreprofullpathrsp flag #165449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e49faac
[lld][COFF] Add /linkreprofullpathrsp flag
DavidTruby Oct 28, 2025
f34b707
Actually find the full path
DavidTruby Nov 3, 2025
5070301
Fix failing test
DavidTruby Nov 3, 2025
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
44 changes: 38 additions & 6 deletions lld/COFF/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,22 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
}
}

void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy) {
static void handleReproFile(COFFLinkerContext &ctx, raw_ostream &reproFile,
StringRef path, bool defaultlib) {
reproFile << '"';
if (defaultlib)
reproFile << "/defaultlib:";
SmallString<128> absPath = path;
std::error_code ec = sys::fs::make_absolute(absPath);
if (ec)
Err(ctx) << "cannot find absolute path for reproFile for " << absPath
<< ": " << ec.message();
sys::path::remove_dots(absPath, true);
reproFile << absPath << "\"\n";
}

void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy,
llvm::raw_ostream *reproFile, bool defaultlib) {
auto future = std::make_shared<std::future<MBErrPair>>(
createFutureForFile(std::string(path)));
std::string pathStr = std::string(path);
Expand Down Expand Up @@ -356,8 +371,14 @@ void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy) {
Err(ctx) << msg;
else
Err(ctx) << msg << "; did you mean '" << nearest << "'";
} else
} else {
// Write full path to library to repro file if /linkreprofullpathrsp
// is specified.
if (reproFile)
handleReproFile(ctx, *reproFile, pathStr, defaultlib);

ctx.driver.addBuffer(std::move(mb), wholeArchive, lazy);
}
});
}

Expand Down Expand Up @@ -514,7 +535,7 @@ void LinkerDriver::parseDirectives(InputFile *file) {
break;
case OPT_defaultlib:
if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
enqueuePath(*path, false, false);
enqueuePath(*path, false, false, nullptr);
Copy link
Member

@aganea aganea Nov 1, 2025

Choose a reason for hiding this comment

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

Why not passing defaultLib = true to the function here?

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025

Choose a reason for hiding this comment

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

I can add it just to avoid confusion, but it doesn't really matter because the reproFile is null here. The defaultlibs get handled again later anyway so I'm not really sure why they're enqueued twice.

Copy link
Member

@aganea aganea Nov 3, 2025

Choose a reason for hiding this comment

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

This is added from a .drectve section which lives in a OBJ. The other handling of /DEFAULTLIB is from the command-line. Libs shouldn't be added twice since we do if (findLibIfNew(info)) before enqueuing it. This my suggestion about moving reproFile in class LinkerDriver.

break;
case OPT_entry:
if (!arg->getValue()[0])
Expand Down Expand Up @@ -2204,6 +2225,17 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->incremental = false;
}

// Handle /linkreprofullpathrsp.
std::unique_ptr<llvm::raw_ostream> reproFile;
Copy link
Member

@aganea aganea Nov 1, 2025

Choose a reason for hiding this comment

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

To avoid passing this around, you can put this in class LinkerDriver and then access it above in LinkerDriver::enqueuePath with ctx.driver.reproFile

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025

Choose a reason for hiding this comment

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

Doing this would make it output the PDB in the .rsp file, which MSVC doesn't do. I think it's easier to leave it as-is so we can control when things get output to the file.

Copy link
Member

@aganea aganea Nov 3, 2025

Choose a reason for hiding this comment

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

See my other comment about the PDB.

if (auto *arg = args.getLastArg(OPT_linkreprofullpathrsp)) {
std::error_code ec;
reproFile = std::make_unique<llvm::raw_fd_ostream>(arg->getValue(), ec);
if (ec) {
Err(ctx) << "cannot open " << arg->getValue() << ": " << ec.message();
reproFile.reset();
}
}

if (errCount(ctx))
return;

Expand Down Expand Up @@ -2245,11 +2277,11 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
break;
case OPT_wholearchive_file:
if (std::optional<StringRef> path = findFileIfNew(arg->getValue()))
enqueuePath(*path, true, inLib);
enqueuePath(*path, true, inLib, reproFile.get());
break;
case OPT_INPUT:
if (std::optional<StringRef> path = findFileIfNew(arg->getValue()))
enqueuePath(*path, isWholeArchive(*path), inLib);
enqueuePath(*path, isWholeArchive(*path), inLib, reproFile.get());
break;
default:
// Ignore other options.
Expand Down Expand Up @@ -2289,7 +2321,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
// addWinSysRootLibSearchPaths(), which is why they are in a separate loop.
for (auto *arg : args.filtered(OPT_defaultlib))
if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
enqueuePath(*path, false, false);
enqueuePath(*path, false, false, reproFile.get(), true);
run();
if (errorCount())
return;
Expand Down
5 changes: 3 additions & 2 deletions lld/COFF/Driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,12 @@ class LinkerDriver {
void enqueueArchiveMember(const Archive::Child &c, const Archive::Symbol &sym,
StringRef parentName);

void enqueuePDB(StringRef Path) { enqueuePath(Path, false, false); }
void enqueuePDB(StringRef Path) { enqueuePath(Path, false, false, nullptr); }
Copy link
Member

@aganea aganea Nov 1, 2025

Choose a reason for hiding this comment

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

The PDB too should be captured in the .rsp I suppose. Otherwise any post-processing script that attemps to extract the inputs from the system will fail to capture every input.

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025

Choose a reason for hiding this comment

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

link.exe doesn't appear to put the PDB in the .rsp, my intention was just to do exactly what link.exe does with this flag.

Copy link
Member

@aganea aganea Nov 3, 2025

Choose a reason for hiding this comment

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

This might be an oversight in the implementation of /LINKREPROFULLPATHRSP in MSVC. We don't have to follow what they do, but what is right. The documentation says "Generates a response file (.RSP) containing the absolute paths of all the files the linker took as input.". The PDB is an input, so it should be there in the .RSP. I can't see how we could make the command-line reproducible otherwise?

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025

Choose a reason for hiding this comment

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

I don't think this is intended to make the command line fully reproducible though; it doesn't include any of the other command line flags, only the paths to libraries. It's possible that it should include the PDBs as well and I'm not necessarily opposed, but I'd need to check if that messes with the main use case for this flag (which is to make it much easier to create Arm64X binaries)


MemoryBufferRef takeBuffer(std::unique_ptr<MemoryBuffer> mb);

void enqueuePath(StringRef path, bool wholeArchive, bool lazy);
void enqueuePath(StringRef path, bool wholeArchive, bool lazy,
raw_ostream *reproFile, bool defaultlib = false);

// Returns a list of chunks of selected symbols.
std::vector<Chunk *> getChunks() const;
Expand Down
3 changes: 3 additions & 0 deletions lld/COFF/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ def libpath : P<"libpath", "Additional library search path">;
def linkrepro : Joined<["/", "-", "/?", "-?"], "linkrepro:">,
MetaVarName<"directory">,
HelpText<"Write repro.tar containing inputs and command to reproduce link">;
def linkreprofullpathrsp : Joined<["/", "-", "/?", "-?"], "linkreprofullpathrsp:">,
MetaVarName<"directory">,
HelpText<"Write .rsp file containing inputs used to link with full paths">;
def lldignoreenv : F<"lldignoreenv">,
HelpText<"Ignore environment variables like %LIB%">;
def lldltocache : P<"lldltocache",
Expand Down
35 changes: 35 additions & 0 deletions lld/test/COFF/linkreprofullpathrsp.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# REQUIRES: x86
# Unsupported on Windows due to maximum path length limitations.
Copy link
Member

@aganea aganea Nov 1, 2025

Choose a reason for hiding this comment

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

Can you explain this comment please?

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025

Choose a reason for hiding this comment

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

I copied this from the other linkrepro tests in this folder, although apparently I forgot to copy the UNSUPPORTED: windows line. I assumed that if this were true for the other tests it would be true for this too.

Copy link
Member

@aganea aganea Nov 3, 2025

Choose a reason for hiding this comment

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

It is unclear why the other tests are maked as unsupported. I don't see anything that justifies that. In the absence of a build error, I would leave this test as "runnable on Windows".

# UNSUPPORTED: system-windows

# RUN: rm -rf %t.dir %t.obj
# RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj

Test link.exe-style /linkreprofullpathrsp: flag.
# RUN: mkdir -p %t.dir/build1
# RUN: cd %t.dir/build1
# RUN: lld-link %t.obj %p/Inputs/std32.lib /subsystem:console /defaultlib:%p/Inputs/library.lib \
# RUN: /libpath:%p/Inputs /defaultlib:std64.lib ret42.lib /entry:main@0 /linkreprofullpathrsp:%t.rsp \
# RUN: /out:%t.exe
# RUN: FileCheck %s --check-prefix=RSP -DT=%t -DP=%p < %t.rsp

# RSP: [[T]].obj
# RSP-NEXT: "[[P]]/Inputs/std32.lib"
# RSP-NEXT: "[[P]]/Inputs/ret42.lib"
# RSP-NEXT: "/defaultlib:[[P]]/Inputs/library.lib"
# RSP-NEXT: "/defaultlib:[[P]]/Inputs/std64.lib"

Copy link
Member

@aganea aganea Nov 1, 2025

Choose a reason for hiding this comment

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

We should probably exercice the link command a second time, by using the generated .rsp to ensure it works as expected. It wouldn't be bad if we checked afterwards some expected outcomes, such as the presence of symbols in the binary as well as the presence of debug info (and maybe ensure no warnings are generated).

Copy link
Member Author

@DavidTruby DavidTruby Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you have any advice on how I could go about doing that? I'm not really a linker expert and this is my first LLD patch in a long while so I'm kinda learning as I'm going 😅

Copy link
Member

@aganea aganea Nov 3, 2025

Choose a reason for hiding this comment

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

If you do lld-link input1.obj input2.obj /listreprofullpathrsp:file.rsp then in a second execution you could do lld-link @file.rsp which should yield the same outcomes (same warnings if any, same outputs).

As for warnings, you can do RSP-NOT: warning to ensure the absence of warnings. Same thing for debug info, assuming that /debug is used during the link phase, you can use llvm-pdbutil dump -all %t.pdb | FileCheck %s ... to ascertain that same important debug information is there. Usually you can run that llvm-pdbutil command manually on your local machine, and copy-paste in the test a few lines that should be important.

#--- drectve.s
.section .drectve, "yn"
.ascii "/defaultlib:std32"

#--- archive.s
.text
.intel_syntax noprefix
.globl exportfn3
.p2align 4
exportfn3:
ret

.section .drectve,"yni"
.ascii " /EXPORT:exportfn3"
Loading