[llvm-ar][libObject] Fix relative paths when nesting thin archives.

[rupprecht]

Summary

When adding one thin archive to another, we currently chop off the relative path to the flattened members. For instance, when adding foo/child.a (which contains x.txt) to parent.a, when flattening it we should add it as foo/x.txt (which exists) instead of x.txt (which does not exist).

As a note, this also undoes the IsNew parameter of handling relative paths in r288280. The unit test there still passes.

This was reported as part of testing the kernel build with llvm-ar: https://patchwork.kernel.org/patch/10767545/ (see the second point).

[ruiu]

llvm/lib/Object/ArchiveWriter.cpp

238

So, M.MemberName is relative to ArcName if it is thin, and it is used as-is if not thin. It doesn't look like we really have to distinguish two here. Could you call computeRelativePath early in llvm-ar and set its result to MemberName? Then I think you can remove special handling of thin archive here.

llvm/test/tools/llvm-ar/flatten-thin-archive-directories.test

15

Are you going to fix this bug as well?

[rupprecht]

llvm/lib/Object/ArchiveWriter.cpp

238

Hmm, good point, that would be a lot simpler. I'll try it.

llvm/test/tools/llvm-ar/flatten-thin-archive-directories.test

15

I'll do it separately -- see D57845

[rupprecht]

• Move computeRelativePath special case from libObject to llvm-ar
• Remove obsolete test fixme

[rupprecht]

llvm/lib/Object/ArchiveWriter.cpp

238

That seems to have worked, thanks for the suggestion!

[ruiu]

LGTM

llvm/lib/Object/ArchiveWriter.cpp

238

This function is now too trivial to be a function. I'd remove this function by replacing a call site with OS << M.MemberName << "/\n";.

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

588

Remove the redundant assignment.

[rupprecht]

• Fix related test to always delete the archive first
• Inline addToStringTable
• Remove redundant assignment

[hans]

I reverted this in r353507 as it broke the Chromium build; see https://crbug.com/930058

[rupprecht]

In D57842#1390249, @hans wrote:

I reverted this in r353507 as it broke the Chromium build; see https://crbug.com/930058

Sorry for the breakage! Thanks for reverting this.

Do you have repro instructions? I'd like to reland this soon.

[rupprecht]

Add regression test for chromium linking issue, and update LibDriver call site to correctly compute relative paths.

[rupprecht]

Reopened + updated patch with LibDriver fixes. I believe this should fix the chromium build, but haven't verified it. The new test case (llvm/test/tools/llvm-lib/thin-relative.test) should capture the issue, and was failing with the old patch but is now fixed.

[hans]

In D57842#1391462, @rupprecht wrote:

Reopened + updated patch with LibDriver fixes. I believe this should fix the chromium build, but haven't verified it. The new test case (llvm/test/tools/llvm-lib/thin-relative.test) should capture the issue, and was failing with the old patch but is now fixed.

Thanks! I'm not the right person to review this, but I can keep an eye on our buildbots when it lands.

[ruiu]

LGTM

llvm/include/llvm/Object/ArchiveWriter.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>
   getOldMember(const object::Archive::Child &OldMember, bool Deterministic);
 
   static Expected<NewArchiveMember> getFile(StringRef FileName,
                                             bool Deterministic);

llvm/lib/Object/ArchiveWriter.cpp

 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();
ErrorOr<std::unique_ptr<MemoryBuffer>> MemberBufferOrErr =
       MemoryBuffer::getOpenFile(FD, FileName, Status.getSize(), false);
   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();
static bool is64BitKind(object::Archive::Kind Kind) {
   case object::Archive::K_GNU64:
     return true;
   }
   llvm_unreachable("not supported for writting");
 }
 
 static void addToStringTable(raw_ostream &Out, StringRef ArcName,
                              const NewArchiveMember &M, bool Thin) {
-  StringRef ID = M.Buf->getBufferIdentifier();
-  if (Thin) {
-    if (M.IsNew)
-      Out << computeRelativePath(ArcName, ID);
-    else
-      Out << ID;
-  } else
+  if (Thin)
+    Out << computeRelativePath(ArcName, M.MemberName);
     Out << M.MemberName;
   Out << "/\n";
 }
 
 static void printMemberHeader(raw_ostream &Out, uint64_t Pos,
                               raw_ostream &StringTable,
                               StringMap<uint64_t> &MemberNames,
                               object::Archive::Kind Kind, bool Thin,
                               StringRef ArcName, const NewArchiveMember &M,
                               sys::TimePoint<std::chrono::seconds> ModTime,
                               unsigned Size) {
-
   if (isBSDLike(Kind))
     return printBSDMemberHeader(Out, Pos, M.MemberName, ModTime, M.UID, M.GID,
                                 M.Perms, Size);
   if (!useStringTable(Thin, M.MemberName))
     return printGNUSmallMemberHeader(Out, M.MemberName, ModTime, M.UID, M.GID,
                                      M.Perms, Size);
   Out << '/';
   uint64_t NamePos;

llvm/test/tools/llvm-ar/flatten-thin-archive-directories.test

+# This test creates a thin archive in a directory and adds it to a thin archive
+# in the parent directory. The relative path should be included when flattening
+# the archive.
+
+RUN: mkdir -p %t/foo
+RUN: touch %t/foo/a.txt
+RUN: rm -f %t/archive.a %t/foo/archive.a
+
+# These tests must be run in the same directory as %t/archive.a. cd %t is
+# included on each line to make debugging this test case easier.
+RUN: cd %t && llvm-ar rcST foo/archive.a foo/a.txt
+RUN: cd %t && llvm-ar rcST archive.a foo/archive.a
+RUN: cd %t && llvm-ar t archive.a | FileCheck %s --match-full-lines
+
+# FIXME: ar t mistakenly prepends / here, this should be "foo/a.txt".
+CHECK: /foo/a.txt

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

 // This variable holds the name of the archive file as given on the
 // command line.
 static std::string ArchiveName;
 
 // This variable holds the list of member files to proecess, as given
 // on the command line.
 static std::vector<StringRef> Members;
 
+// Static buffer to hold StringRefs.
+static BumpPtrAllocator Alloc;
+
 // Extract the member filename from the command line for the [relpos] argument
 // associated with a, b, and i modifiers
 static void getRelPos() {
   if (PositionalArgs.empty())
     fail("Expected [relpos] for a, b, or i modifier");
   RelPos = PositionalArgs[0];
   PositionalArgs.erase(PositionalArgs.begin());
 }

 static void addChildMember(std::vector<NewArchiveMember> &Members,
                            const object::Archive::Child &M,
                            bool FlattenArchive = false) {
   if (Thin && !M.getParent()->isThin())
     fail("Cannot convert a regular archive to a thin one");
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getOldMember(M, Deterministic);
   failIfError(NMOrErr.takeError());
+  if (FlattenArchive) {
+    // The archive child member we're trying to flatten is relative to the
+    // archive it's in, so use the full path here and it will be added as
+    // relative to the archive we're adding it to.
+    StringSaver Saver(Alloc);
+    Expected<std::string> FileNameOrErr = M.getFullName();
+    failIfError(FileNameOrErr.takeError());
+    NMOrErr->MemberName = Saver.save(*FileNameOrErr);
+  }
   if (FlattenArchive &&
       identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
     Expected<std::string> FileNameOrErr = M.getFullName();
     failIfError(FileNameOrErr.takeError());
     object::Archive &Lib = readLibrary(*FileNameOrErr);
     // When creating thin archives, only flatten if the member is also thin.
     if (!Thin || Lib.isThin()) {
       Error Err = Error::success();
Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getFile(FileName, Deterministic);
   failIfError(NMOrErr.takeError(), FileName);
   if (FlattenArchive &&
       identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
     object::Archive &Lib = readLibrary(FileName);
     // When creating thin archives, only flatten if the member is also thin.
     if (!Thin || Lib.isThin()) {
       Error Err = Error::success();
       // Only Thin archives are recursively flattened.
       for (auto &Child : Lib.children(Err))
         addChildMember(Members, Child, /*FlattenArchive=*/Thin);
       failIfError(std::move(Err));
       return;
     }
   }
-  // Use the basename of the object path for the member name.
+  // For regular archives, use the basename of the object path for the member
+  // name. For thin archives, relative paths will be resolved when writing
+  // members, so leave it as the full path here.
+  if (!Thin)
     NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
   Members.push_back(std::move(*NMOrErr));
 }
 
 enum InsertAction {
   IA_AddOldMember,
   IA_AddNewMember,
   IA_Delete,
   IA_MoveOldMember,
for (auto &Child : OldArchive->children(Err)) {
           InsertPos = Pos + 1;
       }
 
       std::vector<StringRef>::iterator MemberI = Members.end();
       InsertAction Action =
           computeInsertAction(Operation, Child, Name, MemberI);
       switch (Action) {
       case IA_AddOldMember:
-        addChildMember(Ret, Child);
+        addChildMember(Ret, Child, /*FlattenArchive=*/Thin);
         break;
       case IA_AddNewMember:
         addMember(Ret, *MemberI);
         break;
       case IA_Delete:
         break;
       case IA_MoveOldMember:
-        addChildMember(Moved, Child);
+        addChildMember(Moved, Child, /*FlattenArchive=*/Thin);
         break;
       case IA_MoveNewMember:
         addMember(Moved, *MemberI);
         break;
       }
       if (MemberI != Members.end())
         Members.erase(MemberI);
     }
if (arg == "-version" || arg == "--version") {
     cl::PrintVersionMessage();
     return true;
   }
   return false;
 }
 
 static int ar_main(int argc, char **argv) {
   SmallVector<const char *, 0> Argv(argv, argv + argc);
-  BumpPtrAllocator Alloc;
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
   for (size_t i = 1; i < Argv.size(); ++i) {
     StringRef Arg = Argv[i];
     const char *match;
     auto MatchFlagWithArg = [&](const char *expected) {
       size_t len = strlen(expected);
       if (Arg == expected) {