[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?

244

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

206–207

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

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

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

llvm/lib/Object/ArchiveWriter.cpp

194

Looks like WriteObjPaths is unused in this function.

[rnk]

llvm/include/llvm/Object/ArchiveWriter.h

41

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

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

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!

clang/lib/Basic/Targets.cpp

void X86TargetInfo::getTargetDefines(const LangOptions &Opts,
 
   if (CPU >= CK_i486) {
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
   }
   if (CPU >= CK_i586)
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
+
+  if (HasFloat128)
+    Builder.defineMacro("__SIZEOF_FLOAT128__", "16");
 }
 
 bool X86TargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
       .Case("aes", HasAES)
       .Case("avx", SSELevel >= AVX)
       .Case("avx2", SSELevel >= AVX2)
       .Case("avx512f", SSELevel >= AVX512F)

clang/lib/Frontend/InitPreprocessor.cpp

#undef TOSTR2
   DefineTypeSizeof("__SIZEOF_SIZE_T__",
                    TI.getTypeWidth(TI.getSizeType()), TI, Builder);
   DefineTypeSizeof("__SIZEOF_WCHAR_T__",
                    TI.getTypeWidth(TI.getWCharType()), TI, Builder);
   DefineTypeSizeof("__SIZEOF_WINT_T__",
                    TI.getTypeWidth(TI.getWIntType()), TI, Builder);
   if (TI.hasInt128Type())
     DefineTypeSizeof("__SIZEOF_INT128__", 128, TI, Builder);
-  if (TI.hasFloat128Type())
-    DefineTypeSizeof("__SIZEOF_FLOAT128__", 128, TI, Builder);
 
   DefineType("__INTMAX_TYPE__", TI.getIntMaxType(), Builder);
   DefineFmt("__INTMAX", TI.getIntMaxType(), TI, Builder);
   Builder.defineMacro("__INTMAX_C_SUFFIX__",
                       TI.getTypeConstantSuffix(TI.getIntMaxType()));
   DefineType("__UINTMAX_TYPE__", TI.getUIntMaxType(), Builder);
   DefineFmt("__UINTMAX", TI.getUIntMaxType(), TI, Builder);
   Builder.defineMacro("__UINTMAX_C_SUFFIX__",

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;
+/// A member needs to use the string table if it is long or if it contains a
+/// slash.
+static bool useStringTable(StringRef Name) {
+  return Name.size() >= 16 || Name.find('/') != StringRef::npos;
 }
 
 static void
-printMemberHeader(raw_fd_ostream &Out, object::Archive::Kind Kind, bool Thin,
-                  StringRef Name,
-                  std::vector<unsigned>::iterator &StringMapIndexIter,
+printMemberHeader(raw_fd_ostream &Out, object::Archive::Kind Kind,
+                  bool WriteObjPaths, StringRef Name,
                   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 (!Name.startswith("/"))
     return printGNUSmallMemberHeader(Out, Name, ModTime, UID, GID, Perms, Size);
-  Out << '/';
-  printWithSpacePadding(Out, *StringMapIndexIter++, 15);
+  printWithSpacePadding(Out, Name, 16);
   printRestOfMemberHeader(Out, ModTime, UID, GID, Perms, Size);
 }
 
 // Compute the relative path from From to To.
 static std::string computeRelativePath(StringRef From, StringRef To) {
   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, "..");
 
   for (auto ToE = sys::path::end(To); ToI != ToE; ++ToI)
     sys::path::append(Relative, *ToI);
 
 #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
 
   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 std::string computeMemberName(StringRef ArchivePath, StringRef ObjPath,
+                                     bool WriteObjPaths, bool Thin) {
+  // 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);
+}
+
+std::vector<std::string>
+computeMemberNames(ArrayRef<NewArchiveMember> NewMembers, StringRef ArcName,
+                   bool WriteObjPaths, bool Thin) {
+  std::vector<std::string> Names;
+  for (const NewArchiveMember &M : NewMembers) {
+    if (M.IsNew) {
+      Names.push_back(computeMemberName(ArcName, M.Buf->getBufferIdentifier(),
+                                        WriteObjPaths, Thin));
+    } else {
+      Names.push_back(M.Buf->getBufferIdentifier());
+    }
+  }
+  return Names;
+}
+
+/// Write a string table. Modify the names in place to contain the names that
+/// will appear in the fixed 16 byte archive member header.
 static void writeStringTable(raw_fd_ostream &Out, StringRef ArcName,
-                             ArrayRef<NewArchiveMember> Members,
-                             std::vector<unsigned> &StringMapIndexes,
-                             bool Thin) {
+                             std::vector<std::string> &MemberNames) {
   unsigned StartOffset = 0;
-  for (const NewArchiveMember &M : Members) {
-    StringRef Path = M.Buf->getBufferIdentifier();
-    StringRef Name = sys::path::filename(Path);
-    if (!useStringTable(Thin, Name))
+  for (std::string &Name : MemberNames) {
+    if (!useStringTable(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;
+    // Compute the short raw name that should be used in the 16 byte member
+    // header.
+    unsigned StringMapIndex = Out.tell() - StartOffset;
+    std::string RawName;
+    RawName = (Twine('/') + Twine(StringMapIndex)).str();
 
-    Out << "/\n";
+    // Print the long name into the string table.
+    Out << Name << "/\n";
+
+    // Replace Name with the short raw name.
+    Name = std::move(RawName);
   }
   if (StartOffset == 0)
     return;
   if (Out.tell() % 2)
     Out << '\n';
   int Pos = Out.tell();
   Out.seek(StartOffset - 12);
   printWithSpacePadding(Out, Pos - StartOffset, 10);
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);
llvm::writeArchive(StringRef ArcName, std::vector<NewArchiveMember> &NewMembers,
   if (WriteSymtab) {
     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;
+  std::vector<std::string> MemberNames =
+      computeMemberNames(NewMembers, ArcName, WriteObjPaths, Thin);
   if (!isBSDLike(Kind))
-    writeStringTable(Out, ArcName, NewMembers, StringMapIndexes, Thin);
+    writeStringTable(Out, ArcName, MemberNames);
 
-  std::vector<unsigned>::iterator StringMapIndexIter = StringMapIndexes.begin();
   std::vector<unsigned> MemberOffset;
-  for (const NewArchiveMember &M : NewMembers) {
+  for (const auto &P : zip(NewMembers, MemberNames)) {
+    const NewArchiveMember &M = std::get<0>(P);
+    const std::string &Name = std::get<1>(P);
     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,
-                      sys::path::filename(M.Buf->getBufferIdentifier()),
-                      StringMapIndexIter, M.ModTime, M.UID, M.GID, M.Perms,
-                      M.Buf->getBufferSize() + Padding);
+    printMemberHeader(Out, Kind, WriteObjPaths, Name, 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)
       Out << '\n';

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=*/false,
+      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/test/Object/archive-format.test

 DARWIN-NEXT: #1/20           0           0     0     644     28        `
 DARWIN-NEXT: 0123456789abcdef{{....}}zed.
 
 
 RUN: rm -f test.a
 RUN: llvm-ar --format=gnu rcT test.a 0123456789abcde 0123456789abcdef
 RUN: cat test.a | FileCheck -strict-whitespace --check-prefix=THIN %s
 THIN: !<thin>
-THIN-NEXT: //                                              36        `
-THIN-NEXT: 0123456789abcde/
+THIN-NEXT: //                                              18        `
 THIN-NEXT: 0123456789abcdef/{{$}}
 THIN: {{^$}}
-THIN: /0              0           0     0     644     4         `
-THIN-NEXT: /17             0           0     0     644     4         `
+THIN: 0123456789abcde/0           0     0     644     4         `
+THIN-NEXT: /0              0           0     0     644     4         `
 
 RUN: mkdir -p bar
 RUN: rm -f bar/test.a
 RUN: llvm-ar --format=gnu rcT bar/test.a 0123456789abcde 0123456789abcdef foo/0123456789abcde foo/0123456789abcdef
 RUN: cat bar/test.a | FileCheck -strict-whitespace --check-prefix=THIN-PATH %s
 THIN-PATH: !<thin>
 THIN-PATH-NEXT: //                                              90        `
 THIN-PATH-NEXT: ..{{/|\\}}0123456789abcde/

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.