[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

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

246

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–209

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–225

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

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

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

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–43

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

llvm/lib/Object/ArchiveWriter.cpp

195

Looks like WriteObjPaths is unused in this function.

[rnk]

llvm/include/llvm/Object/ArchiveWriter.h

41–43

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

195

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

280

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/include/llvm/Object/ArchiveWriter.h

struct NewArchiveMember {
   getOldMember(const object::Archive::Child &OldMember, bool Deterministic);
 
   static Expected<NewArchiveMember> getFile(StringRef FileName,
                                             bool Deterministic);
 };
 
 std::pair<StringRef, std::error_code>
 writeArchive(StringRef ArcName, std::vector<NewArchiveMember> &NewMembers,
-             bool WriteSymtab, object::Archive::Kind Kind, bool Deterministic,
-             bool Thin, std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
+             bool WriteSymtab, bool WriteObjPaths, object::Archive::Kind Kind,
+             bool Deterministic, bool Thin,
+             std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
 }
 
 #endif

llvm/lib/Object/ArchiveWriter.cpp

printBSDMemberHeader(raw_fd_ostream &Out, StringRef Name,
   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;
+static bool useStringTable(bool WriteObjPaths, StringRef Name) {
+  // Force the use of the string table if we're writing full paths.
+  return WriteObjPaths || Name.size() >= 16;
 }
 
 static void
-printMemberHeader(raw_fd_ostream &Out, object::Archive::Kind Kind, bool Thin,
-                  StringRef Name,
+printMemberHeader(raw_fd_ostream &Out, object::Archive::Kind Kind,
+                  bool WriteObjPaths, StringRef Name,
                   std::vector<unsigned>::iterator &StringMapIndexIter,
                   const sys::TimePoint<std::chrono::seconds> &ModTime,
                   unsigned UID, unsigned GID, unsigned Perms, unsigned Size) {
   if (isBSDLike(Kind))
     return printBSDMemberHeader(Out, Name, ModTime, UID, GID, Perms, Size);
-  if (!useStringTable(Thin, Name))
+  if (!useStringTable(WriteObjPaths, Name))
     return printGNUSmallMemberHeader(Out, Name, ModTime, UID, GID, Perms, Size);
   Out << '/';
   printWithSpacePadding(Out, *StringMapIndexIter++, 15);
   printRestOfMemberHeader(Out, ModTime, UID, GID, Perms, Size);
 }
 
 // Compute the relative path from From to To.
-static std::string computeRelativePath(StringRef From, StringRef To) {
+static StringRef computeRelativePath(StringRef From, StringRef To,
+                                     SmallString<128> &Relative) {
   if (sys::path::is_absolute(From) || sys::path::is_absolute(To))
     return To;
 
   StringRef DirFrom = sys::path::parent_path(From);
   auto FromI = sys::path::begin(DirFrom);
   auto ToI = sys::path::begin(To);
   while (*FromI == *ToI) {
     ++FromI;
     ++ToI;
   }
 
-  SmallString<128> Relative;
-  for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI)
-    sys::path::append(Relative, "..");
+  // Use forward slashes everywhere so that our archives look the same on all
+  // platforms.
+  // FIXME: Make it possible for 'lld-link.exe /lib' and 'llvm-lib.exe' to make
+  // archives with backslashes in them.
+  sys::path::Style Style = sys::path::Style::posix;
 
-  for (auto ToE = sys::path::end(To); ToI != ToE; ++ToI)
-    sys::path::append(Relative, *ToI);
+  Relative.clear();
+  for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI)
+    sys::path::append(Relative, Style, "..");
 
-#ifdef LLVM_ON_WIN32
-  // Replace backslashes with slashes so that the path is portable between *nix
-  // and Windows.
-  std::replace(Relative.begin(), Relative.end(), '\\', '/');
-#endif
+  auto ToE = sys::path::end(To);
+  sys::path::append(Relative, ToI, ToE, Style);
 
   return Relative.str();
 }
 
+/// There are three kinds of archive member names:
+/// 1. Plain filename: foo.o
+/// 2. Plain path: lib/bar/foo.o
+/// 3. Relative path from archive for thin archives: ../lib/bar/foo.o
+static StringRef computeMemberName(StringRef ArchivePath, StringRef ObjPath,
+                                   bool WriteObjPaths, bool Thin,
+                                   SmallString<128> &Scratch) {
+  // The non-thin cases are easy: either return the path, or the filename.
+  if (!Thin) {
+    if (WriteObjPaths)
+      return ObjPath;
+    return sys::path::filename(ObjPath);
+  }
+  // In the thin case, compute the relative paths and return that.
+  return computeRelativePath(ArchivePath, ObjPath, Scratch);
+}
+
 static void writeStringTable(raw_fd_ostream &Out, StringRef ArcName,
                              ArrayRef<NewArchiveMember> Members,
                              std::vector<unsigned> &StringMapIndexes,
-                             bool Thin) {
+                             bool Thin, bool WriteObjPaths) {
+  SmallString<128> Scratch;
   unsigned StartOffset = 0;
   for (const NewArchiveMember &M : Members) {
     StringRef Path = M.Buf->getBufferIdentifier();
-    StringRef Name = sys::path::filename(Path);
-    if (!useStringTable(Thin, Name))
+
+    // Leave the member name alone if it isn't new.
+    StringRef Name = Path;
+    if (M.IsNew)
+      Name = computeMemberName(ArcName, Path, WriteObjPaths, Thin, Scratch);
+
+    if (!useStringTable(WriteObjPaths, Name))
       continue;
     if (StartOffset == 0) {
       printWithSpacePadding(Out, "//", 58);
       Out << "`\n";
       StartOffset = Out.tell();
     }
     StringMapIndexes.push_back(Out.tell() - StartOffset);
 
-    if (Thin) {
-      if (M.IsNew)
-        Out << computeRelativePath(ArcName, Path);
-      else
-        Out << M.Buf->getBufferIdentifier();
-    } else
-      Out << Name;
-
-    Out << "/\n";
+    Out << Name << "/\n";
   }
   if (StartOffset == 0)
     return;
   if (Out.tell() % 2)
     Out << '\n';
   int Pos = Out.tell();
   Out.seek(StartOffset - 12);
   printWithSpacePadding(Out, Pos - StartOffset, 10);
   Out.seek(Pos);
 }
 
 static sys::TimePoint<std::chrono::seconds> now(bool Deterministic) {
writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind,
   else
     print32(Out, Kind, NumSyms);
 
   Out.seek(Pos);
   return BodyStartOffset + 4;
 }
 
 std::pair<StringRef, std::error_code>
-llvm::writeArchive(StringRef ArcName,
-                   std::vector<NewArchiveMember> &NewMembers,
-                   bool WriteSymtab, object::Archive::Kind Kind,
-                   bool Deterministic, bool Thin,
+llvm::writeArchive(StringRef ArcName, std::vector<NewArchiveMember> &NewMembers,
+                   bool WriteSymtab, bool WriteObjPaths,
+                   object::Archive::Kind Kind, bool Deterministic, bool Thin,
                    std::unique_ptr<MemoryBuffer> OldArchiveBuf) {
+  if (Thin)
+    assert(WriteObjPaths && "thin archives require paths for archive members");
   assert((!Thin || !isBSDLike(Kind)) && "Only the gnu format has a thin mode");
   SmallString<128> TmpArchive;
   int TmpArchiveFD;
   if (auto EC = sys::fs::createUniqueFile(ArcName + ".temp-archive-%%%%%%%.a",
                                           TmpArchiveFD, TmpArchive))
     return std::make_pair(ArcName, EC);
 
   tool_output_file Output(TmpArchive, TmpArchiveFD);
ErrorOr<unsigned> MemberReferenceOffsetOrErr = writeSymbolTable(
         Out, Kind, NewMembers, MemberOffsetRefs, Deterministic);
     if (auto EC = MemberReferenceOffsetOrErr.getError())
       return std::make_pair(ArcName, EC);
     MemberReferenceOffset = MemberReferenceOffsetOrErr.get();
   }
 
   std::vector<unsigned> StringMapIndexes;
   if (!isBSDLike(Kind))
-    writeStringTable(Out, ArcName, NewMembers, StringMapIndexes, Thin);
+    writeStringTable(Out, ArcName, NewMembers, StringMapIndexes, Thin,
+                     WriteObjPaths);
 
   std::vector<unsigned>::iterator StringMapIndexIter = StringMapIndexes.begin();
   std::vector<unsigned> MemberOffset;
   for (const NewArchiveMember &M : NewMembers) {
     MemoryBufferRef File = M.Buf->getMemBufferRef();
     unsigned Padding = 0;
 
     unsigned Pos = Out.tell();
     MemberOffset.push_back(Pos);
 
     // 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,
+    printMemberHeader(Out, Kind, WriteObjPaths,
                       sys::path::filename(M.Buf->getBufferIdentifier()),
                       StringMapIndexIter, M.ModTime, M.UID, M.GID, M.Perms,
                       M.Buf->getBufferSize() + Padding);
 
     if (!Thin)
       Out << File.getBuffer();
 
     while (Padding--)

llvm/lib/Object/COFFImportFile.cpp

for (COFFShortExport E : Exports) {
     if (!Name) {
       return errorToErrorCode(Name.takeError());
     }
 
     Members.push_back(
         OF.createShortImport(*Name, E.Ordinal, ImportType, NameType));
   }
 
-  std::pair<StringRef, std::error_code> Result =
-      writeArchive(Path, Members, /*WriteSymtab*/ true, object::Archive::K_GNU,
-                   /*Deterministic*/ true, /*Thin*/ false);
-
+  std::pair<StringRef, std::error_code> Result = writeArchive(
+      Path, Members, /*WriteSymtab=*/true, /*WriteObjPaths=*/true,
+      object::Archive::K_GNU, /*Deterministic=*/true, /*Thin=*/false);
   return Result.second;
 }
 
 } // namespace object
 } // namespace llvm

llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp

if (Magic != sys::fs::file_magic::coff_object &&
         Magic != sys::fs::file_magic::windows_resource) {
       llvm::errs() << Arg->getValue()
                    << ": not a COFF object, bitcode or resource file\n";
       return 1;
     }
     Members.emplace_back(std::move(*MOrErr));
   }
 
-  std::pair<StringRef, std::error_code> Result =
-      llvm::writeArchive(getOutputPath(&Args, Members[0]), Members,
-                         /*WriteSymtab=*/true, object::Archive::K_GNU,
-                         /*Deterministic*/ true, Args.hasArg(OPT_llvmlibthin));
+  std::pair<StringRef, std::error_code> Result = llvm::writeArchive(
+      getOutputPath(&Args, Members[0]), Members, /*WriteSymtab=*/true,
+      /*WriteObjPaths=*/true, object::Archive::K_GNU, /*Deterministic=*/true,
+      Args.hasArg(OPT_llvmlibthin));
 
   if (Result.second) {
     if (Result.first.empty())
       Result.first = ArgsArr[0];
     llvm::errs() << Result.first << ": " << Result.second.message() << "\n";
     return 1;
   }
 
   return 0;
 }

llvm/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/tools/llvm-ar/llvm-ar.cpp

case DARWIN:
     if (Thin)
       fail("Only the gnu format has a thin mode");
     Kind = object::Archive::K_DARWIN;
     break;
   }
 
   std::pair<StringRef, std::error_code> Result =
       writeArchive(ArchiveName, NewMembersP ? *NewMembersP : NewMembers, Symtab,
-                   Kind, Deterministic, Thin, std::move(OldArchiveBuf));
+                   /*WriteObjPaths=*/Thin, Kind, Deterministic, Thin,
+                   std::move(OldArchiveBuf));
   failIfError(Result.second, Result.first);
 }
 
 static void createSymbolTable(object::Archive *OldArchive) {
   // When an archive is created or modified, if the s option is given, the
   // resulting archive will have a current symbol table. If the S option
   // is given, it will have no symbol table.
   // In summary, we only need to update the symbol table if we have none.