-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[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.
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
base: main
Are you sure you want to change the base?
Conversation
This patch adds the /linkreprofullpathrsp flag with the same behaviour as link.exe. This flag emits a file containing the full paths to each object passed to the link line. This is used in particular when linking Arm64X binaries, as you need the full path to all the Arm64 objects that were used in a standard Arm64 build. See: https://learn.microsoft.com/en-us/cpp/build/reference/link-repro-full-path-rsp for the Microsoft documentation of the flag.
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: David Truby (DavidTruby) ChangesThis patch adds the /linkreprofullpathrsp flag with the same behaviour as link.exe. This flag emits a file containing the full paths to each object passed to the link line. This is used in particular when linking Arm64X binaries, as you need the full path to all the Arm64 objects that were used in a standard Arm64 build. See: Full diff: https://github.com/llvm/llvm-project/pull/165449.diff 4 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 0e528de9c3652..e2ad2f1c1e993 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -318,7 +318,8 @@ 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,
+ llvm::raw_ostream *reproFile, bool defaultlib) {
auto future = std::make_shared<std::future<MBErrPair>>(
createFutureForFile(std::string(path)));
std::string pathStr = std::string(path);
@@ -356,8 +357,17 @@ 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) {
+ *reproFile << '"';
+ if (defaultlib)
+ *reproFile << "/defaultlib:";
+ *reproFile << pathStr << "\"\n";
+ }
ctx.driver.addBuffer(std::move(mb), wholeArchive, lazy);
+ }
});
}
@@ -514,7 +524,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);
break;
case OPT_entry:
if (!arg->getValue()[0])
@@ -2204,6 +2214,17 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->incremental = false;
}
+ // Handle /linkreprofullpathrsp.
+ std::unique_ptr<llvm::raw_ostream> reproFile;
+ 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;
@@ -2245,11 +2266,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.
@@ -2289,7 +2310,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;
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index 14710d5853bcf..f4b72a317d077 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -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); }
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;
diff --git a/lld/COFF/Options.td b/lld/COFF/Options.td
index d77478fc9c987..dca8a721dc4fd 100644
--- a/lld/COFF/Options.td
+++ b/lld/COFF/Options.td
@@ -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",
diff --git a/lld/test/COFF/linkreprofullpathrsp.test b/lld/test/COFF/linkreprofullpathrsp.test
new file mode 100644
index 0000000000000..c77f09eae9dcd
--- /dev/null
+++ b/lld/test/COFF/linkreprofullpathrsp.test
@@ -0,0 +1,38 @@
+# REQUIRES: x86
+# Unsupported on Windows due to maximum path length limitations.
+
+# RUN: rm -rf %t.dir
+# RUN: split-file %s %t.dir
+# RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj
+# RUN: llvm-mc -filetype=obj -triple=i386-windows %t.dir/drectve.s -o %t.dir/drectve.obj
+# RUN: echo '_main@0' > %t.order
+# RUN: touch %t.def
+# RUN: touch %t.cg
+
+
+
+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: /entry:main@0 /linkreprofullpathrsp:%t.rsp /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: "/defaultlib:[[P]]/Inputs/library.lib"
+
+#--- 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"
|
lld/COFF/Driver.cpp
Outdated
| // Write full path to library to repro file if /linkreprofullpathrsp | ||
| // is specified. | ||
| if (reproFile) { | ||
| *reproFile << '"'; | ||
| if (defaultlib) | ||
| *reproFile << "/defaultlib:"; | ||
| *reproFile << pathStr << "\"\n"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, this will use the path as is, so pathStr may be relative here, which isn’t what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh you're right. I thought I'd checked that and finally found the place where the paths would be full resolved.
I don't suppose you might know offhand where that might be? Otherwise I will keep looking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make the path absolute before writing it in the raw ostream:
SmallString<128> absolutePath = pathStr;
sys::fs::make_absolute(absolutePath);
| } | ||
|
|
||
| // Handle /linkreprofullpathrsp. | ||
| std::unique_ptr<llvm::raw_ostream> reproFile; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| case OPT_defaultlib: | ||
| if (std::optional<StringRef> path = findLibIfNew(arg->getValue())) | ||
| enqueuePath(*path, false, false); | ||
| enqueuePath(*path, false, false, nullptr); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| StringRef parentName); | ||
|
|
||
| void enqueuePDB(StringRef Path) { enqueuePath(Path, false, false); } | ||
| void enqueuePDB(StringRef Path) { enqueuePath(Path, false, false, nullptr); } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused about the PDB files here. I think the reason that link.exe doesn't output them is that the linker doesn't take them as input explicitly (i.e. on the command line), it finds them based on a location specified in the .obj file. So including them would mess up the intended use case for these linkreprofullpathrsp files, which is to be able to do something like: link.exe @file.rsp /some /other /flags. Since the linker doesn't expect to see pdb files there this wouldn't work.
Have I misunderstood something here? I'm really not an expert in how any of this works on Windows so it's highly likely I have...
| @@ -0,0 +1,38 @@ | |||
| # REQUIRES: x86 | |||
| # Unsupported on Windows due to maximum path length limitations. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
| # RUN: echo '_main@0' > %t.order | ||
| # RUN: touch %t.def | ||
| # RUN: touch %t.cg | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra blank lines.
| # RUN: rm -rf %t.dir | ||
| # RUN: split-file %s %t.dir | ||
| # RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj | ||
| # RUN: llvm-mc -filetype=obj -triple=i386-windows %t.dir/drectve.s -o %t.dir/drectve.obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This input doesn't seem to be used; same for the following few lines, they seem to have not effect on the link command below? I think nevertheless that the link command below should test the "/DEFAULTLIB in a .drectve section" interleaved with other inputs; as well as the .PDB input case.
| # RSP: [[T]].obj | ||
| # RSP-NEXT: "[[P]]/Inputs/std32.lib" | ||
| # RSP-NEXT: "/defaultlib:[[P]]/Inputs/library.lib" | ||
|
|
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
This patch adds the /linkreprofullpathrsp flag with the same behaviour as link.exe. This flag emits a file containing the full paths to each object passed to the link line.
This is used in particular when linking Arm64X binaries, as you need the full path to all the Arm64 objects that were used in a standard Arm64 build.
See:
https://learn.microsoft.com/en-us/cpp/build/reference/link-repro-full-path-rsp for the Microsoft documentation of the flag.