[LLD][COFF] Make /wholearchive thin-archive member identifiers consistent #145487
Conversation
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-platform-windows Author: bd1976bris (bd1976bris) ChangesA thin archive is an archive/library format where the archive itself contains only references to member object files on disk, rather than embedding the file contents. For the non-/wholearchive case, we use the path to the archive member as the identifier for thin-archive members (see comments in Apart from consistency, my motivation for fixing this is DTLTO (#126654), where having the member identifier be the path on disk allows distribution of bitcode members during ThinLTO. Full diff: https://github.com/llvm/llvm-project/pull/145487.diff 2 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index f3240b22a1442..caff50d4bbae4 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -275,7 +275,12 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
int memberIndex = 0;
for (MemoryBufferRef m : getArchiveMembers(ctx, archive))
- addArchiveBuffer(m, "<whole-archive>", filename, memberIndex++);
+ if (!archive->isThin())
+ addArchiveBuffer(m, "<whole-archive>", filename, memberIndex++);
+ else
+ // Pass empty string as archive name so that the original filename is
+ // used as the buffer identifier.
+ addArchiveBuffer(m, "<whole-archive>", "", /*OffsetInArchive=*/0);
return;
}
addFile(make<ArchiveFile>(ctx, mbref));
diff --git a/lld/test/COFF/thin-archive.s b/lld/test/COFF/thin-archive.s
index 55d71ea635673..930c7488a7f56 100644
--- a/lld/test/COFF/thin-archive.s
+++ b/lld/test/COFF/thin-archive.s
@@ -22,12 +22,20 @@
# SYMTAB: ?f@@YAHXZ in
# NO-SYMTAB-NOT: ?f@@YAHXZ in
-# RUN: lld-link /entry:main %t.main.obj %t.lib /out:%t.exe 2>&1 | \
-# RUN: FileCheck --allow-empty %s
-# RUN: lld-link /entry:main %t.main.obj %t_thin.lib /out:%t.exe 2>&1 | \
-# RUN: FileCheck --allow-empty %s
-# RUN: lld-link /entry:main %t.main.obj /wholearchive:%t_thin.lib /out:%t.exe 2>&1 | \
-# RUN: FileCheck --allow-empty %s
+# RUN: echo /entry:main %t.main.obj /out:%t.exe /verbose > %t.rsp
+
+# RUN: lld-link @%t.rsp %t.lib 2>&1 | \
+# RUN: FileCheck --allow-empty %s --check-prefixes=CHECK,LOAD_NON_THIN
+# RUN: lld-link @%t.rsp %t_thin.lib 2>&1 | \
+# RUN: FileCheck --allow-empty %s --check-prefixes=CHECK,LOAD_THIN_SYM
+# RUN: lld-link @%t.rsp /wholearchive:%t_thin.lib 2>&1 | \
+# RUN: FileCheck --allow-empty %s --check-prefixes=CHECK,LOAD_THIN_WHOLE
+# RUN: lld-link @%t.rsp /wholearchive %t_thin.lib 2>&1 | \
+# RUN: FileCheck --allow-empty %s --check-prefixes=CHECK,LOAD_THIN_WHOLE
+
+# LOAD_NON_THIN: Loaded thin-archive{{.*}}.lib(thin-archive{{.*}}.obj) for int __cdecl f(void)
+# LOAD_THIN_SYM: Loaded {{.*}}thin-archive{{.*}}.obj for int __cdecl f(void)
+# LOAD_THIN_WHOLE: Loaded {{.*}}thin-archive{{.*}}.obj for <whole-archive>
# RUN: rm %t.lib.obj
# RUN: lld-link /entry:main %t.main.obj %t.lib /out:%t.exe 2>&1 | \
|
bd1976bris
force-pushed
the
dtlto_coff_fix_whole_thin_archives
branch
3 times, most recently
from
89db92c to
661eddd
Compare
…tent A thin archive is an archive/library format where the archive itself contains only references to member object files on disk, rather than embedding the file contents. For the non-/wholearchive case, we use the path to the archive member as the identifier for thin-archive members (see comments in `enqueueArchiveMember`). This patch modifies the /wholearchive path to behave the same way. Apart from consistency, my motivation for fixing this is DTLTO (llvm#126654), where having the member identifier be the path on disk allows distribution of bitcode members during ThinLTO.
bd1976bris
force-pushed
the
dtlto_coff_fix_whole_thin_archives
branch
from
661eddd to
7c8c158
Compare
|
Rebased and force-pushed to base this patch on a more recent commit. |
|
ping. |
|
LGTM, but we probably need someone else to review as well. Make sure to backport this to 21.x as well so that we have all the PR's for the feature there. cc @aganea |
This comment was marked as spam.
This comment was marked as spam.
lld/COFF/Driver.cpp
Outdated
| @@ -275,7 +275,12 @@ void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb, | |||
|
|
|||
| int memberIndex = 0; | |||
| for (MemoryBufferRef m : getArchiveMembers(ctx, archive)) | |||
There was a problem hiding this comment.
I would add braces to the for to enclose the code below.
lld/COFF/Driver.cpp
Outdated
| addArchiveBuffer(m, "<whole-archive>", filename, memberIndex++); | ||
| else | ||
| // Pass empty string as archive name so that the original filename is | ||
| // used as the buffer identifier. |
There was a problem hiding this comment.
If you could also add a short comment about why we're setting 0 for the offset, as you did in the description, it'd be great.
There was a problem hiding this comment.
Done. I now use a common function to add thin archive members so that I don't have to duplicate this comment in two places.
|
@aganea thanks very much for reviewing. I have addressed your comments now. I also made a few minor improvements to the test. |
…aining parameter choice.
bd1976bris
force-pushed
the
dtlto_coff_fix_whole_thin_archives
branch
from
8df4e6c to
ba737e0
Compare
A thin archive is an archive/library format where the archive itself contains only references to member object files on disk, rather than embedding the file contents.
For the non-/wholearchive case, we use the path to the archive member as the identifier for thin-archive members (see comments in
enqueueArchiveMember). This patch modifies the /wholearchive path to behave the same way.Apart from consistency, my motivation for fixing this is DTLTO (#126654), where having the member identifier be the path on disk allows distribution of bitcode members during ThinLTO.