Access ADDLIB in llvm-ar via command line

[gbreynoo]

Summary

ADDLIB is called to add the contents of an archive to another archive. This is currently only available in llvm-ar and gnu-ar through the use of an MRI script. We propose adding a new modifier to the llvm-ar command line so workflows that require the ADDLIB functionality are not forced to use MRI scripts.

With the use of a new "L" modifier, archive files can treated in the manner above when using quick append.

"L" was chosen as a key letter only because it is not used by gnu-ar, any alternate suggestions would be appreciated.

[mstorsjo]

Other than the minor comment I had, this looks sensible to me. Do we have any policy or predecent on adding this kind of custom additions to tools like ar from before? (I'd like to at least have some sort of ack from someone else to approve this.)

tools/llvm-ar/llvm-ar.cpp

553

Any reason why this path can't just call the default addMember?

[gbreynoo]

tools/llvm-ar/llvm-ar.cpp

553

It would mean repeating the getFile() call, I wasn't sure it was worth the overhead.

[mstorsjo]

tools/llvm-ar/llvm-ar.cpp

553

Ah, I see. That makes sense to me.

[gbreynoo]

Ping.

[mstorsjo]

In D53332#1275585, @gbreynoo wrote:

Ping.

Still looks sensible to me, but I'd like some sort of ack from at least someone else, since we're extending the interface of an otherwise standardized tool.

[rupprecht]

Seems reasonable to use L given that gnu-ar doesn't mention it/POSIX spec doesn't say it's reserved for anything. I'm fine with any other letter that fits that requirement too.

It would mean that anything using this would no longer be buildable w/ a GNU toolchain, but that's true of any other reason someone might be motivated to switch to an llvm binutils toolchain. Requiring MRI scripts for portable libraries doesn't seem too bad.

[MaskRay]

tools/llvm-ar/llvm-ar.cpp

681

I think { } is redundant as there is another case below that does not add { } for simple statement like this..

[MaskRay]

Hi after a revision is accepted, you can run arc amend to update Reviewed By: Reviewers: lines from Phabricator to local git commit description.

Make sure your git commit has a line Differential Revision: ... to associate it with the Phabricator revision, so that when you commit the revision can be automatically closed.

[jyknight]

tools/llvm-ar/llvm-ar.cpp

99

Maybe say "For ar archives in [files], add each member, rather than the archive itself." or something along those lines, to indicate that both .a and .o files are handled with L.

344

Seems a bit odd that it doesn't work with 'r'?

[rupprecht]

Manually closing this -- it was committed in rL345383

test/tools/llvm-ar/Inputs/add-lib1.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+  - Name:            .comment
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    AddressAlign:    0x0000000000000001
+    Content:         00636C616E672076657273696F6E20332E392E3020287472756E6B203237333632342920286C6C766D2F7472756E6B203237333633362900
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+Symbols:
+  Global:
+    - Name:     lib1
+      Index:    SHN_ABS
+      Value:    0x1234
+  Local:
+    - Name:            '-'
+      Type:            STT_FILE
+...

test/tools/llvm-ar/Inputs/add-lib2.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+  - Name:            .comment
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    AddressAlign:    0x0000000000000001
+    Content:         00636C616E672076657273696F6E20332E392E3020287472756E6B203237333632342920286C6C766D2F7472756E6B203237333633362900
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+Symbols:
+  Global:
+    - Name:     lib2
+      Index:    SHN_ABS
+      Value:    0x1234
+  Local:
+    - Name:            '-'
+      Type:            STT_FILE
+...

test/tools/llvm-ar/Inputs/add-lib3.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+  - Name:            .comment
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_MERGE, SHF_STRINGS ]
+    AddressAlign:    0x0000000000000001
+    Content:         00636C616E672076657273696F6E20332E392E3020287472756E6B203237333632342920286C6C766D2F7472756E6B203237333633362900
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+Symbols:
+  Global:
+    - Name:     lib3
+      Index:    SHN_ABS
+      Value:    0x1234
+  Local:
+    - Name:            '-'
+      Type:            STT_FILE
+...

test/tools/llvm-ar/add-library.test

+RUN: yaml2obj %S/Inputs/add-lib1.yaml -o %t-add-lib1.o
+RUN: yaml2obj %S/Inputs/add-lib2.yaml -o %t-add-lib2.o
+RUN: yaml2obj %S/Inputs/add-lib2.yaml -o %t-add-lib3.o
+
+RUN: rm -f %t.ar
+RUN: llvm-ar crs %t.ar %t-add-lib1.o
+RUN: llvm-ar cqs %t.ar %t-add-lib2.o
+
+RUN: llvm-ar tv %t.ar | FileCheck %s --check-prefix=CHECK-NAMES-NO-ADDLIB
+CHECK-NAMES-NO-ADDLIB: add-library.test.tmp-add-lib1.o
+CHECK-NAMES-NO-ADDLIB: add-library.test.tmp-add-lib2.o
+
+RUN: llvm-nm %t.ar | FileCheck %s --check-prefix=CHECK-SYMBOLS-NO-ADDLIB
+CHECK-SYMBOLS-NO-ADDLIB: add-lib1
+CHECK-SYMBOLS-NO-ADDLIB: add-lib2
+
+RUN: rm -f %t1.ar
+RUN: llvm-ar crs %t1.ar %t-add-lib3.o
+RUN: llvm-ar cqLs %t1.ar %t.ar
+
+RUN: llvm-ar tv %t1.ar | FileCheck %s --check-prefix=CHECK-NAMES-ADDLIB
+CHECK-NAMES-ADDLIB: add-library.test.tmp-add-lib3.o
+CHECK-NAMES-ADDLIB: add-library.test.tmp-add-lib1.o
+CHECK-NAMES-ADDLIB: add-library.test.tmp-add-lib2.o
+
+RUN: llvm-nm %t1.ar | FileCheck %s --check-prefix=CHECK-SYMBOLS-ADDLIB
+CHECK-SYMBOLS-ADDLIB: add-lib3
+CHECK-SYMBOLS-ADDLIB: add-lib1
+CHECK-SYMBOLS-ADDLIB: add-lib2
+
+RUN: llvm-ar cqLs %t1.ar %t-add-lib1.o
+
+RUN: llvm-ar tv %t1.ar | FileCheck %s --check-prefix=CHECK-NAMES-DUPLICATE
+CHECK-NAMES-DUPLICATE: add-library.test.tmp-add-lib3.o
+CHECK-NAMES-DUPLICATE: add-library.test.tmp-add-lib1.o
+CHECK-NAMES-DUPLICATE: add-library.test.tmp-add-lib2.o
+CHECK-NAMES-DUPLICATE: add-library.test.tmp-add-lib1.o
+
+RUN: llvm-nm %t1.ar | FileCheck %s --check-prefix=CHECK-SYMBOLS-DUPLICATE
+CHECK-SYMBOLS-DUPLICATE: add-lib3
+CHECK-SYMBOLS-DUPLICATE: add-lib1
+CHECK-SYMBOLS-DUPLICATE: add-lib2
+CHECK-SYMBOLS-DUPLICATE: add-lib1

tools/llvm-ar/llvm-ar.cpp

 
 MODIFIERS:
   [a] - put [files] after [relpos]
   [b] - put [files] before [relpos] (same as [i])
   [c] - do not warn if archive had to be created
   [D] - use zero for timestamps and uids/gids (default)
   [i] - put [files] before [relpos] (same as [b])
   [l] - ignored for compatibility
+  [L] - add archive's contents
   [o] - preserve original dates
   [s] - create an archive index (cf. ranlib)
   [S] - do not build a symbol table
   [T] - create a thin archive
   [u] - update only [files] newer than archive contents
   [U] - use actual timestamps and uids/gids
   [v] - be verbose about actions taken
 )";

 static bool AddBefore = false;     ///< 'b' modifier
 static bool Create = false;        ///< 'c' modifier
 static bool OriginalDates = false; ///< 'o' modifier
 static bool OnlyUpdate = false;    ///< 'u' modifier
 static bool Verbose = false;       ///< 'v' modifier
 static bool Symtab = true;         ///< 's' modifier
 static bool Deterministic = true;  ///< 'D' and 'U' modifiers
 static bool Thin = false;          ///< 'T' modifier
+static bool AddLibrary = false;    ///< 'L' modifier
 
 // Relative Positional Argument (for insert/move). This variable holds
 // the name of the archive member to which the 'a', 'b' or 'i' modifier
 // refers. Only one of 'a', 'b' or 'i' can be specified so we only need
 // one variable.
 static std::string RelPos;
 
 // This variable holds the name of the archive file as given on the

 }
 
 // Copy over remaining items in PositionalArgs to our Members vector
 static void getMembers() {
   for (auto &Arg : PositionalArgs)
     Members.push_back(Arg);
 }
 
+std::vector<std::unique_ptr<MemoryBuffer>> ArchiveBuffers;
+std::vector<std::unique_ptr<object::Archive>> Archives;
+
+static object::Archive &readLibrary(const Twine &Library) {
+  auto BufOrErr = MemoryBuffer::getFile(Library, -1, false);
+  failIfError(BufOrErr.getError(), "Could not open library");
+  ArchiveBuffers.push_back(std::move(*BufOrErr));
+  auto LibOrErr =
+      object::Archive::create(ArchiveBuffers.back()->getMemBufferRef());
+  failIfError(errorToErrorCode(LibOrErr.takeError()),
+              "Could not parse library");
+  Archives.push_back(std::move(*LibOrErr));
+  return *Archives.back();
+}
+
 static void runMRIScript();
 
 // Parse the command line options as presented and return the operation
 // specified. Process all modifiers and check to make sure that constraints on
 // modifier/operation pairs have not been violated.
 static ArchiveOperation parseCommandLine() {
   if (MRI) {
     if (!PositionalArgs.empty() || !Options.empty())
case 'D':
       Deterministic = true;
       break;
     case 'U':
       Deterministic = false;
       break;
     case 'T':
       Thin = true;
       break;
+    case 'L':
+      AddLibrary = true;
+      break;
     default:
       fail(std::string("unknown option ") + Options[i]);
     }
   }
 
   // At this point, the next thing on the command line must be
   // the archive name.
   getArchive();
if (AddAfter || AddBefore) {
     if (Operation != Move && Operation != ReplaceOrInsert)
       fail("The 'a', 'b' and 'i' modifiers can only be specified with "
            "the 'm' or 'r' operations");
   }
   if (OriginalDates && Operation != Extract)
     fail("The 'o' modifier is only applicable to the 'x' operation");
   if (OnlyUpdate && Operation != ReplaceOrInsert)
     fail("The 'u' modifier is only applicable to the 'r' operation");
+  if (AddLibrary && Operation != QuickAppend)
+    fail("The 'L' modifier is only applicable to the 'q' operation");
 
   // Return the parsed operation to the caller
   return Operation;
 }
 
 // Implements the 'p' operation. This function traverses the archive
 // looking for members that match the path list.
 static void doPrint(StringRef Name, const object::Archive::Child &C) {
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);
 }
 
+static void addLibMember(std::vector<NewArchiveMember> &Members,
+                         StringRef FileName) {
+  Expected<NewArchiveMember> NMOrErr =
+      NewArchiveMember::getFile(FileName, Deterministic);
+  failIfError(NMOrErr.takeError(), FileName);
+  if (identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
+    object::Archive &Lib = readLibrary(FileName);
+    Error Err = Error::success();
+
+    for (auto &Child : Lib.children(Err))
+      addMember(Members, Child);
+
+    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));
+  }
+}
+
 enum InsertAction {
   IA_AddOldMember,
   IA_AddNewMember,
   IA_Delete,
   IA_MoveOldMember,
   IA_MoveNewMember
 };
 
computeNewArchiveMembers(ArchiveOperation Operation,
 
   assert(unsigned(InsertPos) <= Ret.size());
   int Pos = InsertPos;
   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);
+    }
+    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;
   }
 

 static void runMRIScript() {
   enum class MRICommand { AddLib, AddMod, Create, Delete, Save, End, Invalid };
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf = MemoryBuffer::getSTDIN();
   failIfError(Buf.getError());
   const MemoryBuffer &Ref = *Buf.get();
   bool Saved = false;
   std::vector<NewArchiveMember> NewMembers;
-  std::vector<std::unique_ptr<MemoryBuffer>> ArchiveBuffers;
-  std::vector<std::unique_ptr<object::Archive>> Archives;
 
   for (line_iterator I(Ref, /*SkipBlanks*/ false), E; I != E; ++I) {
     StringRef Line = *I;
     Line = Line.split(';').first;
     Line = Line.split('*').first;
     Line = Line.trim();
     if (Line.empty())
       continue;
     StringRef CommandStr, Rest;
     std::tie(CommandStr, Rest) = Line.split(' ');
     Rest = Rest.trim();
     if (!Rest.empty() && Rest.front() == '"' && Rest.back() == '"')
       Rest = Rest.drop_front().drop_back();
     auto Command = StringSwitch<MRICommand>(CommandStr.lower())
                        .Case("addlib", MRICommand::AddLib)
                        .Case("addmod", MRICommand::AddMod)
                        .Case("create", MRICommand::Create)
                        .Case("delete", MRICommand::Delete)
                        .Case("save", MRICommand::Save)
                        .Case("end", MRICommand::End)
                        .Default(MRICommand::Invalid);
 
     switch (Command) {
     case MRICommand::AddLib: {
-      auto BufOrErr = MemoryBuffer::getFile(Rest, -1, false);
-      failIfError(BufOrErr.getError(), "Could not open library");
-      ArchiveBuffers.push_back(std::move(*BufOrErr));
-      auto LibOrErr =
-          object::Archive::create(ArchiveBuffers.back()->getMemBufferRef());
-      failIfError(errorToErrorCode(LibOrErr.takeError()),
-                  "Could not parse library");
-      Archives.push_back(std::move(*LibOrErr));
-      object::Archive &Lib = *Archives.back();
+      object::Archive &Lib = readLibrary(Rest);
       {
         Error Err = Error::success();
         for (auto &Member : Lib.children(Err))
           addMember(NewMembers, Member);
         failIfError(std::move(Err));
       }
       break;
     }