[llvm-ar] Flatten thin archives.

[rupprecht]

Summary

Normal behavior for GNU ar is to flatten thin archives when adding them to another thin archive, i.e. add the members directly instead of nesting the archive.

Some refactoring done as part of this patch to ease things:

• Consolidate addMember/addLibMember methods
• Rename addMember to addChildMember to make it more visibly different at the call site that an archive child is passed instead of a regular member
• Pass in a separate vector and splice it back into position instead of passing a vector + optional Pos (which makes expanding libs tricky)

This fixes PR37530 as raised by https://github.com/ClangBuiltLinux/linux/issues/279.

[mstorsjo]

LGTM

[ruiu]

LGTM

It looks like that behavior is a bit odd, but that's exactly what GNU ar does according to GNU's man page.

[void]

This fails if the archives are nested. E.g.:

$ mkdir -p foo/bar
$ cat > foo/bar/a.c <<EOF
int bar() { return 42; }
EOF
$ rm -f built-in.o foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD foo/bar/built-in.o foo/bar/a.o ; llvm-ar rcSTD foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD built-in.o foo/built-in.o
./llvm.opt.install/bin/llvm-ar: error: 'a.o': No such file or directory.

What's happening is that it's trying to find a.o in foo/ when it's really in foo/bar/.

[rupprecht]

In D56508#1397156, @void wrote:

This fails if the archives are nested. E.g.:

$ mkdir -p foo/bar
$ cat > foo/bar/a.c <<EOF
int bar() { return 42; }
EOF
$ rm -f built-in.o foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD foo/bar/built-in.o foo/bar/a.o ; llvm-ar rcSTD foo/built-in.o foo/bar/built-in.o ; llvm-ar rcSTD built-in.o foo/built-in.o
./llvm.opt.install/bin/llvm-ar: error: 'a.o': No such file or directory.

What's happening is that it's trying to find a.o in foo/ when it's really in foo/bar/.

That should be fixed by D57842, which I'm planning to submit soon -- I want to poke a few more test cases first.

[void]

In D56508#1397173, @rupprecht wrote:

In D56508#1397156, @void wrote:

That should be fixed by D57842, which I'm planning to submit soon -- I want to poke a few more test cases first.

Great! Thank you.

test/tools/llvm-ar/Inputs/a-plus-b.a

+!<thin>
+//                                              14        `
+a.txt/
+b.txt/
+/0              0           0     0     644     11        `
+/7              0           0     0     644     11        `

test/tools/llvm-ar/Inputs/a.txt

+a-contents

test/tools/llvm-ar/Inputs/b.txt

+b-contents

test/tools/llvm-ar/Inputs/c.txt

+c-contents

test/tools/llvm-ar/Inputs/d.txt

+d-contents

test/tools/llvm-ar/Inputs/nested-thin-archive.a

+!<thin>
+//                                              20        `
+a-plus-b.a/
+c.txt/
+
+/0              0           0     0     644     202       `
+/12             0           0     0     644     11        `

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

+# Since llvm-ar cannot create thin archives that contain any thin archives,
+# nested-thin-archive.a is a manually constructed thin archive that contains
+# another (unflattened) thin archive.
+# This test ensures that flat archives are recursively flattened.
+
+RUN: rm -f %t.a
+RUN: llvm-ar rcsT %t.a %S/Inputs/nested-thin-archive.a %S/Inputs/d.txt
+RUN: llvm-ar t %t.a | FileCheck %s
+
+CHECK:      a.txt
+CHECK-NEXT: b.txt
+CHECK-NEXT: c.txt
+CHECK-NEXT: d.txt

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

+# This test creates a thin archive that contains a thin archive, a regular
+# archive, and a file.
+#
+# The inner thin archive should be flattened, but the regular archive should
+# not. The order of members in the archive should match the input order, with
+# flattened members appearing together.
+
+RUN: touch %t-a.txt %t-b.txt %t-c.txt %t-d.txt %t-e.txt
+RUN: rm -f %t-a-plus-b.a %t.a
+RUN: llvm-ar rcsT %t-a-plus-b.a %t-a.txt %t-b.txt
+RUN: llvm-ar rcs %t-d-plus-e.a %t-d.txt %t-e.txt
+RUN: llvm-ar rcsT %t.a %t-a-plus-b.a %t-c.txt %t-d-plus-e.a
+RUN: llvm-ar t %t.a | FileCheck %s
+
+CHECK:      a.txt
+CHECK-NEXT: b.txt
+CHECK-NEXT: c.txt
+CHECK-NEXT: -d-plus-e.a

tools/llvm-ar/llvm-ar.cpp

static void performReadOperation(ArchiveOperation Operation,
 
   if (Members.empty())
     return;
   for (StringRef Name : Members)
     errs() << Name << " was not found\n";
   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) {
+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 (Pos == -1)
-    Members.push_back(std::move(*NMOrErr));
-  else
-    Members[Pos] = std::move(*NMOrErr);
+  if (FlattenArchive &&
+      identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
+    Expected<StringRef> FileNameOrErr = M.getName();
+    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();
+      // Only Thin archives are recursively flattened.
+      for (auto &Child : Lib.children(Err))
+        addChildMember(Members, Child, /*FlattenArchive=*/Thin);
+      failIfError(std::move(Err));
+      return;
+    }
+  }
 }
 
-static void addLibMember(std::vector<NewArchiveMember> &Members,
-                         StringRef FileName) {
+static void addMember(std::vector<NewArchiveMember> &Members,
+                      StringRef FileName, bool FlattenArchive = false) {
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getFile(FileName, Deterministic);
   failIfError(NMOrErr.takeError(), FileName);
-  if (identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
+  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))
-      addMember(Members, Child);
-
+        addChildMember(Members, Child, /*FlattenArchive=*/Thin);
       failIfError(std::move(Err));
-  } else {
-    // Use the basename of the object path for the member name.
-    NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
-    Members.push_back(std::move(*NMOrErr));
-  }
-}
+      return;
+    }
+  }
 
 enum InsertAction {
   IA_AddOldMember,
   IA_AddNewMember,
   IA_Delete,
   IA_MoveOldMember,
   IA_MoveNewMember
 };
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:
-        addMember(Ret, Child);
+        addChildMember(Ret, Child);
         break;
       case IA_AddNewMember:
         addMember(Ret, *MemberI);
         break;
       case IA_Delete:
         break;
       case IA_MoveOldMember:
-        addMember(Moved, Child);
+        addChildMember(Moved, Child);
         break;
       case IA_MoveNewMember:
         addMember(Moved, *MemberI);
         break;
       }
       if (MemberI != Members.end())
         Members.erase(MemberI);
     }
computeNewArchiveMembers(ArchiveOperation Operation,
   for (auto &M : Moved) {
     Ret.insert(Ret.begin() + Pos, std::move(M));
     ++Pos;
   }
 
   if (AddLibrary) {
     assert(Operation == QuickAppend);
     for (auto &Member : Members)
-      addLibMember(Ret, Member);
+      addMember(Ret, Member, /*FlattenArchive=*/true);
     return Ret;
   }
 
-  for (unsigned I = 0; I != Members.size(); ++I)
-    Ret.insert(Ret.begin() + InsertPos, NewArchiveMember());
-  Pos = InsertPos;
-  for (auto &Member : Members) {
-    addMember(Ret, Member, Pos);
-    ++Pos;
-  }
+  std::vector<NewArchiveMember> NewMembers;
+  for (auto &Member : Members)
+    addMember(NewMembers, Member, /*FlattenArchive=*/Thin);
+  Ret.reserve(Ret.size() + NewMembers.size());
+  std::move(NewMembers.begin(), NewMembers.end(),
+            std::inserter(Ret, std::next(Ret.begin(), InsertPos)));
 
   return Ret;
 }
 
 static object::Archive::Kind getDefaultForHost() {
   return Triple(sys::getProcessTriple()).isOSDarwin()
              ? object::Archive::K_DARWIN
              : object::Archive::K_GNU;
auto Command = StringSwitch<MRICommand>(CommandStr.lower())
                        .Default(MRICommand::Invalid);
 
     switch (Command) {
     case MRICommand::AddLib: {
       object::Archive &Lib = readLibrary(Rest);
       {
         Error Err = Error::success();
         for (auto &Member : Lib.children(Err))
-          addMember(NewMembers, Member);
+          addChildMember(NewMembers, Member);
         failIfError(std::move(Err));
       }
       break;
     }
     case MRICommand::AddMod:
       addMember(NewMembers, Rest);
       break;
     case MRICommand::Create: