[llvm] [llvm-lib] Don't rewrite paths for members in non-thin archives (PR #123416)

[Martin Storsjö via llvm-commits]

https://github.com/mstorsjo created https://github.com/llvm/llvm-project/pull/123416

This matches what MS lib.exe does (and llvm-ar too); when adding files to an archive, MS lib.exe stores the file name as it was given on the command line, whereas llvm-lib rewrote it into a relative path name, relative to the archive location. Such a rewrite makes sense for thin archives, but not for regular archives. (MS lib.exe doesn't support producing thin archives; that's an LLVM extension - see the thin-relative.test testcase.)

The behaviour to rewrite these paths was added in 451c2ef199e9c5163007ac32e2d426fbfb37e664; it is unclear why it was chosen to do the rewriting for non-thin archives as well. This quirk is even pointed out in a code comment - but neither the code review at https://reviews.llvm.org/D57842 nor the linked bug report at https://crbug.com/41440160 mentions why this is done for all archives, not only thin ones.

Therefore, assume that this only was done out of convenience, and change llvm-lib to not adjust the paths for non-thin archives.

Normally, the actual member names doesn't matter for non-thin archives; however for short import libraries, where each member is named e.g. "foo.dll", the names do matter. If using llvm-lib to merge two import libraries (as a non-thin library), preserve the original names rather than making the member names relative.

>From bc993f4572d34d54e7aa64cc1dc5c10e7725478b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin at martin.st>
Date: Fri, 17 Jan 2025 16:49:57 +0200
Subject: [PATCH] [llvm-lib] Don't rewrite paths for non-thin archives

This matches what MS lib.exe does (and llvm-ar too); MS lib.exe
doesn't produce relative paths when producing regular static
archives. (lib.exe doesn't support producing thin archives; that's
an LLVM extension; see the thin-relative.test testcase.)

It is unclear why 451c2ef199e9c5163007ac32e2d426fbfb37e664 chose
to extend this behaviour to non-thin archives in the case of llvm-lib
(and even mentioning it in a comment), while not doing that for
llvm-ar; the code review at https://reviews.llvm.org/D57842 or the
linked bug report at https://crbug.com/41440160 doesn't touch upon
that.

Therefore, assume that this only was done out of convenience, and
change llvm-lib to not adjust the paths for non-thin archives.

Normally, the actual member names doesn't matter for non-thin
archives; however for short import libraries, where each member
is named e.g. "foo.dll", the names do matter. If using llvm-lib
to merge two import libraries (as a non-thin library), preserve
the original names rather than making the member names relative.
---
 llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp | 21 ++++++------
 llvm/test/tools/llvm-lib/member-names.test  | 37 +++++++++++++++++++++
 2 files changed, 47 insertions(+), 11 deletions(-)
 create mode 100644 llvm/test/tools/llvm-lib/member-names.test

diff --git a/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp b/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
index 6ce06b434b2c05..15d959d7712dd7 100644
--- a/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
+++ b/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
@@ -503,23 +503,22 @@ int llvm::libDriverMain(ArrayRef<const char *> ArgsArr) {
       return 1;
     }
   }
-  // llvm-lib uses relative paths for both regular and thin archives, unlike
-  // standard GNU ar, which only uses relative paths for thin archives and
-  // basenames for regular archives.
-  for (NewArchiveMember &Member : Members) {
-    if (sys::path::is_relative(Member.MemberName)) {
-      Expected<std::string> PathOrErr =
-          computeArchiveRelativePath(OutputPath, Member.MemberName);
-      if (PathOrErr)
-        Member.MemberName = Saver.save(*PathOrErr);
+
+  bool Thin = Args.hasArg(OPT_llvmlibthin);
+  if (Thin) {
+    for (NewArchiveMember &Member : Members) {
+      if (sys::path::is_relative(Member.MemberName)) {
+        Expected<std::string> PathOrErr =
+            computeArchiveRelativePath(OutputPath, Member.MemberName);
+        if (PathOrErr)
+          Member.MemberName = Saver.save(*PathOrErr);
+      }
     }
   }
 
   // For compatibility with MSVC, reverse member vector after de-duplication.
   std::reverse(Members.begin(), Members.end());
 
-  bool Thin = Args.hasArg(OPT_llvmlibthin);
-
   auto Symtab = Args.hasFlag(OPT_llvmlibindex, OPT_llvmlibindex_no,
                              /*default=*/true)
                     ? SymtabWritingMode::NormalSymtab
diff --git a/llvm/test/tools/llvm-lib/member-names.test b/llvm/test/tools/llvm-lib/member-names.test
new file mode 100644
index 00000000000000..75437a02f30a2f
--- /dev/null
+++ b/llvm/test/tools/llvm-lib/member-names.test
@@ -0,0 +1,37 @@
+RUN: rm -rf %t
+RUN: mkdir -p %t/foo
+RUN: split-file %s %t
+RUN: cd %t
+
+RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o foo/obj.o %S/Inputs/a.s
+
+# For a regular, non-thin archive, check that we store the path in the form
+# it was passed (foo/obj.o), not as a path relative to the archive.
+RUN: llvm-lib -out:foo/regular.a foo/obj.o
+RUN: llvm-lib -list foo/regular.a | FileCheck %s --check-prefix=REGULAR --match-full-lines
+REGULAR: foo/obj.o
+
+# When merging two import libraries, make sure that the member names stay
+# unchanged.
+RUN: llvm-lib -machine:x64 -out:foo.lib -def:foo.def
+RUN: llvm-lib -machine:x64 -out:bar.lib -def:bar.def
+RUN: llvm-lib -out:foo/merged.lib foo.lib bar.lib
+RUN: llvm-lib -list foo/merged.lib | FileCheck %s --check-prefix=MERGED --match-full-lines
+MERGED: foo.dll
+MERGED: foo.dll
+MERGED: foo.dll
+MERGED: foo.dll
+MERGED: bar.dll
+MERGED: bar.dll
+MERGED: bar.dll
+MERGED: bar.dll
+
+#--- foo.def
+LIBRARY foo.dll
+EXPORTS
+        func1
+
+#--- bar.def
+LIBRARY bar.dll
+EXPORTS
+        func2