[llvm-ar] Make llvm-lib behave more like the MSVC archiver

[rnk]

Summary

Use the filepath used to open the archive member as the archive member
name instead of the file basename. This path might be absolute or
relative. This is important because the archive member name will show
up in the PDB, and we want our PDBs to look as much like MSVC's as
possible.

This also helps avoid an issue in our PDB module descriptor writing
code, which assumes that all module names are unique. Relative paths
still aren't guaranteed to be unique, but they're much better than
basenames, which definitely aren't unique.

[ruiu]

llvm/lib/Object/ArchiveWriter.cpp

187 ↗

(On Diff #100325)

If it fits in 16 byte buffer, we can write there even if a path contains a path separator, no?

246 ↗

(On Diff #100325)

Debug output?

[zturner]

Why do we even need WriteObjPaths? Can't we just standardize on true for writing, but still support false for reading?

llvm/lib/Object/ArchiveWriter.cpp

207–208 ↗

(On Diff #100325)

For some reason I thought we had an llvm function to do this, essentially mimicing the behavior of os.path.commonprefix in python. I guess I was wrong though.

220–222 ↗

(On Diff #100325)

Is this what we want? If we're producing an archive to be used on Windows, it seems like we would want it to be as close to MSVC's behavior as possible, which is going to be to use backslashes (on Windows anyway). It seems like this could confuse MSVC's linker if we feed it archives that have forward slashes.

224 ↗

(On Diff #100325)

I don't think this is entirely what you want. This means more than just "use forward slashes". It means "use the entire posix path grammar". Valid filename characters, drive letter parsing, etc all depend on this. I suspect we'll run into subtle changes with this. If you really just want to use slashes, use the native host path style, and at the end write replace(str, '\\', '/');

[rnk]

llvm/lib/Object/ArchiveWriter.cpp

187 ↗

(On Diff #100325)

I don't think so. '/' seems to be a special character inside the short name.

[rnk]

• remove errs prints

[rnk]

llvm/lib/Object/ArchiveWriter.cpp

224 ↗

(On Diff #100325)

That's awful. I'll go ahead and revert my changes here. I just wanted to get rid of #ifdef LLVM_ON_WIN32. :(

[rnk]

• revert path append changes

[rnk]

• Compute all member names up front

[rnk]

• Preserve old member names when updating archives

[pcc]

llvm/include/llvm/Object/ArchiveWriter.h

41 ↗

(On Diff #100468)

Would it be simpler to store the member name in NewArchiveMember and pass WriteObjPaths to NewArchiveMember::getFile()?

llvm/lib/Object/ArchiveWriter.cpp

194 ↗

(On Diff #100468)

Looks like WriteObjPaths is unused in this function.

[rnk]

llvm/include/llvm/Object/ArchiveWriter.h

41 ↗

(On Diff #100468)

We'd still need to re-relativize that path for thin archives during writeArchive, though. I like the way the name computation logic is all in the same place with the current patch.

llvm/lib/Object/ArchiveWriter.cpp

194 ↗

(On Diff #100468)

True, we can get rid of it now.

[rnk]

• remove dead bool

[rnk]

ping

[ruiu]

I wonder how many uses of this class in LLVM. I'm asking because it feels more cleaner to me if you remove WriteObjPaths parameter from writeArchive and strip directories if it's needed on the client side.

llvm/lib/Object/ArchiveWriter.cpp

290 ↗

(On Diff #100493)

This could be

(Twine(/) + Twine(StringMapIndex)).str();

?

[rnk]

Rewrite the patch to store the name in NewArchiveMember

[rnk]

I rewrote the patch to store the name in the NewArchiveMember. I think initially I avoided this approach because it duplicates the member name between the MemoryBuffer and the NewArchiveMember, and it still isn't the final name for thin archives. I kind of liked centralizing the member name calculation logic in writeArchive. However, looking at it now, this definitely feels cleaner. :)

[ruiu]

LGTM

This looks much better!

llvm/trunk/include/llvm/Object/ArchiveWriter.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/FileSystem.h"
 
 namespace llvm {
 
 struct NewArchiveMember {
   std::unique_ptr<MemoryBuffer> Buf;
+  StringRef MemberName;
   sys::TimePoint<std::chrono::seconds> ModTime;
   unsigned UID = 0, GID = 0, Perms = 0644;
 
   bool IsNew = false;
   NewArchiveMember() = default;
   NewArchiveMember(MemoryBufferRef BufRef);
 
   static Expected<NewArchiveMember>

llvm/trunk/lib/Object/ArchiveWriter.cpp

 #include <unistd.h>
 #else
 #include <io.h>
 #endif
 
 using namespace llvm;
 
 NewArchiveMember::NewArchiveMember(MemoryBufferRef BufRef)
-    : Buf(MemoryBuffer::getMemBuffer(BufRef, false)) {}
+    : Buf(MemoryBuffer::getMemBuffer(BufRef, false)),
+      MemberName(BufRef.getBufferIdentifier()) {}
 
 Expected<NewArchiveMember>
 NewArchiveMember::getOldMember(const object::Archive::Child &OldMember,
                                bool Deterministic) {
   Expected<llvm::MemoryBufferRef> BufOrErr = OldMember.getMemoryBufferRef();
   if (!BufOrErr)
     return BufOrErr.takeError();
 
   NewArchiveMember M;
   assert(M.IsNew == false);
   M.Buf = MemoryBuffer::getMemBuffer(*BufOrErr, false);
+  M.MemberName = M.Buf->getBufferIdentifier();
   if (!Deterministic) {
     auto ModTimeOrErr = OldMember.getLastModified();
     if (!ModTimeOrErr)
       return ModTimeOrErr.takeError();
     M.ModTime = ModTimeOrErr.get();
     Expected<unsigned> UIDOrErr = OldMember.getUID();
     if (!UIDOrErr)
       return UIDOrErr.takeError();
if (!MemberBufferOrErr)
     return errorCodeToError(MemberBufferOrErr.getError());
 
   if (close(FD) != 0)
     return errorCodeToError(std::error_code(errno, std::generic_category()));
 
   NewArchiveMember M;
   M.IsNew = true;
   M.Buf = std::move(*MemberBufferOrErr);
+  M.MemberName = M.Buf->getBufferIdentifier();
   if (!Deterministic) {
     M.ModTime = std::chrono::time_point_cast<std::chrono::seconds>(
         Status.getLastModificationTime());
     M.UID = Status.getUser();
     M.GID = Status.getGroup();
     M.Perms = Status.permissions();
   }
   return std::move(M);
printRestOfMemberHeader(Out, ModTime, UID, GID, Perms,
                           NameWithPadding + Size);
   Out << Name;
   assert(PosAfterHeader == Out.tell());
   while (Pad--)
     Out.write(uint8_t(0));
 }
 
 static bool useStringTable(bool Thin, StringRef Name) {
-  return Thin || Name.size() >= 16;
+  return Thin || Name.size() >= 16 || Name.contains('/');
 }
 
 static void
 printMemberHeader(raw_fd_ostream &Out, object::Archive::Kind Kind, bool Thin,
                   StringRef Name,
                   std::vector<unsigned>::iterator &StringMapIndexIter,
                   const sys::TimePoint<std::chrono::seconds> &ModTime,
                   unsigned UID, unsigned GID, unsigned Perms, unsigned Size) {

 
 static void writeStringTable(raw_fd_ostream &Out, StringRef ArcName,
                              ArrayRef<NewArchiveMember> Members,
                              std::vector<unsigned> &StringMapIndexes,
                              bool Thin) {
   unsigned StartOffset = 0;
   for (const NewArchiveMember &M : Members) {
     StringRef Path = M.Buf->getBufferIdentifier();
-    StringRef Name = sys::path::filename(Path);
+    StringRef Name = M.MemberName;
     if (!useStringTable(Thin, Name))
       continue;
     if (StartOffset == 0) {
       printWithSpacePadding(Out, "//", 58);
       Out << "`\n";
       StartOffset = Out.tell();
     }
     StringMapIndexes.push_back(Out.tell() - StartOffset);
for (const NewArchiveMember &M : NewMembers) {
 
     // ld64 expects the members to be 8-byte aligned for 64-bit content and at
     // least 4-byte aligned for 32-bit content.  Opt for the larger encoding
     // uniformly.  This matches the behaviour with cctools and ensures that ld64
     // is happy with archives that we generate.
     if (Kind == object::Archive::K_DARWIN)
       Padding = OffsetToAlignment(M.Buf->getBufferSize(), 8);
 
-    printMemberHeader(Out, Kind, Thin,
-                      sys::path::filename(M.Buf->getBufferIdentifier()),
-                      StringMapIndexIter, M.ModTime, M.UID, M.GID, M.Perms,
+    printMemberHeader(Out, Kind, Thin, M.MemberName, StringMapIndexIter,
+                      M.ModTime, M.UID, M.GID, M.Perms,
                       M.Buf->getBufferSize() + Padding);
 
     if (!Thin)
       Out << File.getBuffer();
 
     while (Padding--)
       Out << '\n';
     if (Out.tell() % 2)

llvm/trunk/test/LibDriver/use-paths.test

+llvm-lib should behave like "link.exe /lib" and use relative paths to describe
+archive members.
+
+First, get in a clean working directory.
+RUN: rm -rf %t && mkdir -p %t && cd %t
+
+Make foo/a.obj and foo/b.obj.
+RUN: mkdir foo
+RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o foo/a.obj %S/Inputs/a.s
+RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o foo/b.obj %S/Inputs/b.s
+
+RUN: llvm-lib -out:foo.lib foo/a.obj foo/b.obj
+RUN: llvm-ar t foo.lib | FileCheck %s
+
+FIXME: We should probably use backslashes on Windows to better match MSVC tools.
+CHECK: foo/a.obj
+CHECK: foo/b.obj
+
+Do it again with absolute paths and see that we get something.
+RUN: llvm-lib -out:foo.lib %t/foo/a.obj %t/foo/b.obj
+RUN: llvm-ar t foo.lib | FileCheck %s --check-prefix=ABS
+
+ABS: {{.*}}/foo/a.obj
+ABS: {{.*}}/foo/b.obj

llvm/trunk/tools/llvm-ar/llvm-ar.cpp

static void performReadOperation(ArchiveOperation Operation,
   exit(1);
 }
 
 static void addMember(std::vector<NewArchiveMember> &Members,
                       StringRef FileName, int Pos = -1) {
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getFile(FileName, Deterministic);
   failIfError(NMOrErr.takeError(), FileName);
+
+  // Use the basename of the object path for the member name.
+  NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
+
   if (Pos == -1)
     Members.push_back(std::move(*NMOrErr));
   else
     Members[Pos] = std::move(*NMOrErr);
 }
 
 static void addMember(std::vector<NewArchiveMember> &Members,
                       const object::Archive::Child &M, int Pos = -1) {