[lld/COFF] Handle -start-lib / -end-lib better in /reproduce: output #119752
Conversation
Previously, we'd collect all input files in Driver::filePaths, and then write filePaths after all other flags in createResponseFile(). This meant that `-start-lib foo.obj -end-lib` would be written as `-start-lib -end-lib foo.obj`, changing semantics. Instead, remove Driver::filePaths, and handle things that fed into it directly: * OPT_INPUT is now handled in the same way as other flags, so that we now get `-start-lib foo.obj -end-lib` in response.txt as desired. Add a test for -start-lib / -end-lib and /reproduce:. * OPT_wholearchive_file needs explicit handling now -- but before, this was buggy as well: We'd put the flag without a rewritten path in response.txt, but also the rewritten input file without wholearchive semantics via filePaths. So this commit makes --whole-archive work with /reproduce: too, and adds test coverage. * /defaultlib:foo is now written as /defaultlib:foo into response.txt, instead of writing the resolved path previously. While response.txt looks slightly differently, both should have the same semantics, and this should be mostly a no-op. (It does require updating a test.) * /defaultlib: from .drectve sections are no longer recorded in response.txt. This seems like a progression -- in the non-repro case they come from .obj files, so they should come (only) from there in the repro case too. This adds test coverage for this case. Makes createResponseFile() look more like the versions in the ELF and MachO ports too.
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Nico Weber (nico) ChangesPreviously, we'd collect all input files in Driver::filePaths, and then write filePaths after all other flags in Instead, remove Driver::filePaths, and handle things that fed into it directly:
Makes createResponseFile() look more like the versions in the ELF and MachO ports too. Full diff: https://github.com/llvm/llvm-project/pull/119752.diff 3 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 714de67e88b065..43287530d8e3d4 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -204,7 +204,6 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
StringRef filename = mb->getBufferIdentifier();
MemoryBufferRef mbref = takeBuffer(std::move(mb));
- filePaths.push_back(filename);
// File type is detected by contents, not by file extension.
switch (identify_magic(mbref.getBuffer())) {
@@ -857,7 +856,6 @@ static std::string rewritePath(StringRef s) {
// Reconstructs command line arguments so that so that you can re-run
// the same command with the same inputs. This is for --reproduce.
static std::string createResponseFile(const opt::InputArgList &args,
- ArrayRef<StringRef> filePaths,
ArrayRef<StringRef> searchPaths) {
SmallString<0> data;
raw_svector_ostream os(data);
@@ -866,11 +864,15 @@ static std::string createResponseFile(const opt::InputArgList &args,
switch (arg->getOption().getID()) {
case OPT_linkrepro:
case OPT_reproduce:
- case OPT_INPUT:
- case OPT_defaultlib:
case OPT_libpath:
case OPT_winsysroot:
break;
+ case OPT_INPUT:
+ os << quote(rewritePath(arg->getValue())) << "\n";
+ break;
+ case OPT_wholearchive_file:
+ os << arg->getSpelling() << quote(rewritePath(arg->getValue())) << "\n";
+ break;
case OPT_call_graph_ordering_file:
case OPT_deffile:
case OPT_manifestinput:
@@ -907,9 +909,6 @@ static std::string createResponseFile(const opt::InputArgList &args,
os << "/libpath:" << quote(relPath) << "\n";
}
- for (StringRef path : filePaths)
- os << quote(relativeToRoot(path)) << "\n";
-
return std::string(data);
}
@@ -2348,7 +2347,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
if (tar) {
llvm::TimeTraceScope timeScope("Reproducer: response file");
tar->append("response.txt",
- createResponseFile(args, filePaths,
+ createResponseFile(args,
ArrayRef<StringRef>(searchPaths).slice(1)));
}
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index 3889feb7511c0a..71577aec3a199d 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -195,7 +195,6 @@ class LinkerDriver {
bool run();
std::list<std::function<void()>> taskQueue;
- std::vector<StringRef> filePaths;
std::vector<MemoryBufferRef> resources;
llvm::DenseSet<StringRef> directivesExports;
diff --git a/lld/test/COFF/linkrepro.test b/lld/test/COFF/linkrepro.test
index d5a34a201a5da4..a5779a9ec82e16 100644
--- a/lld/test/COFF/linkrepro.test
+++ b/lld/test/COFF/linkrepro.test
@@ -1,7 +1,10 @@
# REQUIRES: x86, shell
# 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
@@ -46,7 +49,7 @@ and various other flags.
# RUN: diff %t.order repro/%:t.order
# RUN: diff %t.def repro/%:t.def
# RUN: diff %p/Inputs/std32.lib repro/%:p/Inputs/std32.lib
-# RUN: FileCheck %s --check-prefix=RSP < repro/response.txt
+# RUN: FileCheck %s --check-prefix=RSP-DEFAULTLIB < repro/response.txt
# RUN: cd repro; lld-link @response.txt
Test adding .lib files with LIB env var to repro archive,
@@ -61,7 +64,7 @@ and various other flags.
# RUN: diff %t.order repro/%:t.order
# RUN: diff %t.def repro/%:t.def
# RUN: diff %p/Inputs/std32.lib repro/%:p/Inputs/std32.lib
-# RUN: FileCheck %s --check-prefix=RSP < repro/response.txt
+# RUN: FileCheck %s --check-prefix=RSP-DEFAULTLIB < repro/response.txt
# RUN: cd repro; lld-link @response.txt
# LIST: .obj
@@ -70,14 +73,21 @@ and various other flags.
# LIST: .def
# LIST: .order
+# RSP: linkrepro.test.tmp.obj
+# RSP: std32.lib
# RSP: /subsystem:console
# RSP: /entry:main@0
# RSP: /out:
# RSP-NOT: /order:@/
# RSP-NOT: /def:/
-# RSP: linkrepro.test.tmp.obj
-# RSP-NOT: defaultlib
-# RSP: std32.lib
+
+# RSP-DEFAULTLIB: linkrepro.test.tmp.obj
+# RSP-DEFAULTLIB: /defaultlib:std32
+# RSP-DEFAULTLIB: /subsystem:console
+# RSP-DEFAULTLIB: /entry:main@0
+# RSP-DEFAULTLIB: /out:
+# RSP-DEFAULTLIB-NOT: /order:@/
+# RSP-DEFAULTLIB-NOT: /def:/
Test /call-graph-ordering-file (can't be used with /order:, needs separate test)
# RUN: mkdir -p %t.dir/build5
@@ -96,3 +106,90 @@ Test /call-graph-ordering-file (can't be used with /order:, needs separate test)
# LISTCG: .cg
# RSPCG-NOT: /call-graph-ordering-file:/
+
+Test /defaultlib: from a .drectve section
+# RUN: mkdir -p %t.dir/build6
+# RUN: cd %t.dir/build6
+# RUN: lld-link %t.obj %t.dir/drectve.obj /libpath:%p/Inputs /subsystem:console \
+# RUN: /entry:main@0 /linkrepro:. -safeseh:no /out:%t.exe /order:@%t.order /def:%t.def
+# RUN: tar tf repro.tar | FileCheck --check-prefix=LIST %s
+# RUN: tar xf repro.tar
+# RUN: diff %t.obj repro/%:t.obj
+# RUN: diff %t.order repro/%:t.order
+# RUN: diff %t.def repro/%:t.def
+# RUN: diff %p/Inputs/std32.lib repro/%:p/Inputs/std32.lib
+# RUN: FileCheck %s --check-prefix=RSP-DRECTVE < repro/response.txt
+# RUN: cd repro; lld-link @response.txt
+
+# RSP-DRECTVE: linkrepro.test.tmp.obj
+# RSP-DRECTVE: drectve.obj
+# RSP-DRECTVE: /subsystem:console
+# RSP-DRECTVE: /entry:main@0
+# RSP-DRECTVE: -safeseh:no
+# RSP-DRECTVE: /out:
+
+Test /wholearchive: with /linkrepro:
+# RUN: llvm-mc -filetype=obj -triple=i386-windows %t.dir/archive.s -o %t.dir/archive.obj
+# RUN: rm -f %t.dir/build7/archive.lib
+# RUN: llvm-ar rcs %t.dir/archive.lib %t.dir/archive.obj
+# RUN: mkdir -p %t.dir/build7
+# RUN: cd %t.dir/build7
+ RUN: lld-link %t.obj /defaultlib:std32 /libpath:%p/Inputs /subsystem:console \
+# RUN: /entry:main@0 /linkrepro:. -safeseh:no /wholearchive:%t.dir/archive.lib \
+# RUN: /out:%t.exe /order:@%t.order /def:%t.def
+# RUN: tar tf repro.tar | FileCheck --check-prefix=LIST %s
+# RUN: tar xf repro.tar
+# RUN: diff %t.obj repro/%:t.obj
+# RUN: diff %t.order repro/%:t.order
+# RUN: diff %t.def repro/%:t.def
+# RUN: diff %p/Inputs/std32.lib repro/%:p/Inputs/std32.lib
+# RUN: FileCheck %s --check-prefix=RSP-WHOLEARCHIVE < repro/response.txt
+# RUN: cd repro; lld-link @response.txt
+
+# RSP-WHOLEARCHIVE: linkrepro.test.tmp.obj
+# RSP-WHOLEARCHIVE: /defaultlib:std32
+# RSP-WHOLEARCHIVE: /subsystem:console
+# RSP-WHOLEARCHIVE: /entry:main@0
+# RSP-WHOLEARCHIVE: -safeseh:no
+# RSP-WHOLEARCHIVE: /wholearchive:{{.*}}archive.lib
+# RSP-WHOLEARCHIVE: /out:
+
+Test /start-lib / /end-lib with /linkrepro:
+# RUN: mkdir -p %t.dir/build8
+# RUN: cd %t.dir/build8
+# RUN: lld-link %t.obj /defaultlib:std32 /libpath:%p/Inputs /subsystem:console \
+# RUN: /entry:main@0 /linkrepro:. -safeseh:no /start-lib %t.dir/drectve.obj /end-lib \
+# RUN: /out:%t.exe /order:@%t.order /def:%t.def
+# RUN: tar tf repro.tar | FileCheck --check-prefix=LIST %s
+# RUN: tar xf repro.tar
+# RUN: diff %t.obj repro/%:t.obj
+# RUN: diff %t.order repro/%:t.order
+# RUN: diff %t.def repro/%:t.def
+# RUN: diff %p/Inputs/std32.lib repro/%:p/Inputs/std32.lib
+# RUN: FileCheck %s --check-prefix=RSP-STARTLIB < repro/response.txt
+# RUN: cd repro; lld-link @response.txt
+
+# RSP-STARTLIB: linkrepro.test.tmp.obj
+# RSP-STARTLIB: /defaultlib:std32
+# RSP-STARTLIB: /subsystem:console
+# RSP-STARTLIB: /entry:main@0
+# RSP-STARTLIB: -safeseh:no
+# RSP-STARTLIB: /start-lib
+# RSP-STARTLIB-NEXT: drectve.obj
+# RSP-STARTLIB-NEXT: /end-lib
+# RSP-STARTLIB: /out:
+
+#--- 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"
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
LGTM, this looks like a nice simplification! I wonder why it wasn't done like this from the get-go.
I remember running into some problems with some arguments around reproduce outputs as well, let's see if I remember them, now when you're digging around here :-)
With PDB outputs, I think the repro output has the problem that it will try to write the output into a directory that might not exist; this probably will need to be rewritten into something like just /pdb:out.pdb rather than the original PDB output path.
Then for really tricky things; if you have static libraries involved, that actually are thin libraries, i.e. just referencing other object files on disk, those aren't included in the repro package. And even if they would be, the static library would need to be rewritten to reference them with their relocatable name within the repro bundle.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/10494 Here is the relevant piece of the build log for the reference |
Thanks!
That sounds right.
lld/test/ELF/reproduce-thin-archive.s lld/test/MachO/reproduce-thin-archive-objc.s lld/test/MachO/reproduce-thin-archives.s all exist, so someone seems to have thought about this in the past. We should probably do the same in the COFF port, if we aren't yet :) |
It me! 😮 https://reviews.llvm.org/D92456 I think /reproduce sets up a sysroot, so maybe changing path isn't even necessary as long as we include the files? I don't remember what I was thinking back then, but I think it did work. The behavior for thin archives without an index is also different across ports. If I find time, I'd like to look at that, so I might look at thin archives + lld/COFF too. |
|
Future self (or whoever gets to it): Also do #97169 for lld/COFF. |
=> #121512 |
Previously, we'd collect all input files in Driver::filePaths, and then write filePaths after all other flags in
createResponseFile(). This meant that
-start-lib foo.obj -end-libwould be written as-start-lib -end-lib foo.obj, changing semantics.Instead, remove Driver::filePaths, and handle things that fed into it directly:
OPT_INPUT is now handled in the same way as other flags, so that we now get
-start-lib foo.obj -end-libin response.txt as desired. Add a test for -start-lib / -end-lib and /reproduce:.OPT_wholearchive_file needs explicit handling now -- but before, this was buggy as well: We'd put the flag without a rewritten path in response.txt, but also the rewritten input file without wholearchive semantics via filePaths. So this commit makes --whole-archive work with /reproduce: too, and adds test coverage.
/defaultlib:foo is now written as /defaultlib:foo into response.txt, instead of writing the resolved path previously. While response.txt looks slightly differently, both should have the same semantics, and this should be mostly a no-op. (It does require updating a test.)
/defaultlib: from .drectve sections are no longer recorded in response.txt. This seems like a progression -- in the non-repro case they come from .obj files, so they should come (only) from there in the repro case too. This adds test coverage for this case.
Makes createResponseFile() look more like the versions in the ELF and MachO ports too.