[llvm-libtool-darwin] Allow flattening archives

[sameerarora101]

Summary

Add support for flattening archives while creating static libraries. As
per cctools' libtool's behavior, llvm-libtool-darwin does not flatten
archives recursively. Depends on D83002.

[sameerarora101]

Updating type and comments

[sameerarora101]

verifyDarwinObject -> verifyMachOObject

[sameerarora101]

MachO -> Mach-O in comments

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/invalid-archive.test

4 ↗

(On Diff #277423)

Instead of using a separate Inputs file, you might want to consider using echo to create the input file on the fly. That has the advantage of keeping the test self-contained - you don't have to go looking elsewhere to understand what exactly is going on.

9 ↗

(On Diff #277423)

I take it this behaviour is taken from the original libtool?

12 ↗

(On Diff #277423)

Are you deliberately trying to add the archive symbol index here? That is done implicitly by llvm-ar anyway, so cr is sufficient.

17 ↗

(On Diff #277423)

This needs fixing (you are deleting the archive from the previous case, not the two archives from this case).

19–20 ↗

(On Diff #277423)

Ditto.

27–31 ↗

(On Diff #277423)

Consider using a different archive file name here, so that you don't overwrite the archive from the earlier case. This can be useful in some debugging situations.

llvm/test/tools/llvm-libtool-darwin/valid-archive.test

1 ↗

(On Diff #277423)

I'd rename this test archive-flattening.test. You should also fold in the invalid cases into it, as there's no need to keep them separate.

7 ↗

(On Diff #277423)

What's the reasoning for deleting %t.lib explicitly here?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

98

Don't use auto here. It's not obvious what the type is.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/invalid-archive.test

9 ↗

(On Diff #277423)

Yup, cctools' libtool also throws an error for this case

llvm/test/tools/llvm-libtool-darwin/valid-archive.test

7 ↗

(On Diff #277423)

Oh yeah, I don't need to do that as llvm-libtool-darwin overwrites it. Removed now. Thanks

[sameerarora101]

Merge tests into archive-flattening.test

[jhenderson]

LGTM, with the test case reordering. If you want to wait for D83834 too, then that's fine by me.

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

6

I'd reverse the order - put the positive cases before the negative ones.

7–8

It's not committed at the time of writing, but D83834 adds a utility to split an input file into smaller pieces. It might be an even nicer way than using echo to emit the archive.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

7–8

Ok, I can wait for it (while I also need to wait for few others to review this diff) Thanks!

[sameerarora101]

Putting positive tests before negative ones.

[smeenai]

LGTM

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

28

The archive format is more properly referred to as "Darwin" (at least that's the terminology LLVM uses); the object file format is Mach-O.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

28

ok, got it! Thanks

[sameerarora101]

Mach-O -> Darwin for archive format in comments

[smeenai]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

97–102

Thinking through this a little more, Err is passed to the iterator to indicate iteration issues, so you should be checking it inside the loop in each iteration (probably before you call addChildMember). You still need to do an explicit check of Err afterward, so that in the case where the loop didn't iterate at all, the Error is still considered checked (it's guaranteed to be a success in that case, but you still need to do an explicit boolean check to mark it as checked). Perhaps the following would suffice for that? I'm not sure what pattern other parts of the codebase use for this.

static_cast<bool>(Err);

[smeenai]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

97–102

Ah, never mind. The iteration will stop in case of an error, so having the error check after the loop is perfectly fine. (See llvm/include/llvm/ADT/fallible_iterator.)

[jhenderson]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

97–102

I had basically this same thought originally too, but this is the pattern for looping through archive members elsewhere, so I realised it was safe.

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

+## This test checks that an archive is flattened correctly.
+
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
+# RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o
+
+## Cannot read archive:
+# RUN: echo '!<arch>' >  %t-invalid-archive.lib
+# RUN: echo 'invalid' >> %t-invalid-archive.lib
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-invalid-archive.lib 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-ARCHIVE -DARCHIVE=%t-invalid-archive.lib
+
+# INVALID-ARCHIVE: error: '[[ARCHIVE]]': truncated or malformed archive
+
+## Archive member not an object file:
+# RUN: rm -f %t.not-object.ar
+# RUN: touch %t.txt
+# RUN: llvm-ar cr %t.not-object.ar %t.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.not-object.ar 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-OBJECT -DARCHIVE=%t.not-object.ar -DFILE=%basename_t.tmp.txt
+
+## Do not recursively flatten archives:
+# RUN: rm -f %t.inner
+# RUN: rm -f %t.outer
+# RUN: llvm-ar cr %t.inner %t-input1.o
+# RUN: llvm-ar cr %t.outer %t.inner
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.outer 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-OBJECT -DARCHIVE=%t.outer -DFILE=%basename_t.tmp.inner
+
+# NOT-OBJECT: error: '[[ARCHIVE]]': '[[FILE]]': The file was not recognized as a valid object file
+
+## Archive member not a Mach-O object file:
+# RUN: rm -f %t.not-macho.ar
+# RUN: yaml2obj %s -o %t.elf
+# RUN: llvm-ar cr %t.not-macho.ar %t.elf
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.not-macho.ar 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-MACHO -DARCHIVE=%t.not-macho.ar -DFILE=%basename_t.tmp.elf
+
+# NOT-MACHO: error: '[[ARCHIVE]]': '[[FILE]]': format not supported
+
+## Input a correct archive:
+# RUN: rm -f %t.correct.ar
+# RUN: llvm-ar cr %t.correct.ar %t-input1.o %t-input2.o
+# RUN: llvm-libtool-darwin -static -o %t.lib %t.correct.ar
+
+## Check that binaries are present:
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+
+# CHECK-NAMES:      [[PREFIX]]-input1.o
+# CHECK-NAMES-NEXT: [[PREFIX]]-input2.o
+
+## Check that symbols are present:
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# CHECK-SYMBOLS:      Archive map
+# CHECK-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# CHECK-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# CHECK-SYMBOLS-EMPTY:
+
+## Check that output archive is in Mach-O format:
+# RUN: llvm-objdump --macho --archive-headers %t.lib | \
+# RUN:   FileCheck %s --check-prefix=FORMAT -DPREFIX=%basename_t.tmp -DARCHIVE=%t.lib
+
+# FORMAT:      Archive : [[ARCHIVE]]
+# FORMAT-NEXT: __.SYMDEF
+# FORMAT-NEXT: [[PREFIX]]-input1.o
+# FORMAT-NEXT: [[PREFIX]]-input2.o
+# FORMAT-NOT:  {{.}}
+
+## Passing both archive and object file:
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input2.o %t.correct.ar %t-input1.o
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=BOTH-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=BOTH-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# BOTH-NAMES:      [[PREFIX]]-input2.o
+# BOTH-NAMES-NEXT: [[PREFIX]]-input1.o
+# BOTH-NAMES-NEXT: [[PREFIX]]-input2.o
+# BOTH-NAMES-NEXT: [[PREFIX]]-input1.o
+
+# BOTH-SYMBOLS:      Archive map
+# BOTH-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# BOTH-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# BOTH-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# BOTH-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# BOTH-SYMBOLS-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64

llvm/tools/llvm-libtool-darwin/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  BinaryFormat
   Object
   Support
   )
 
 add_llvm_tool(llvm-libtool-darwin
   llvm-libtool-darwin.cpp
 )

llvm/tools/llvm-libtool-darwin/LLVMBuild.txt

 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 [component_0]
 type = Tool
 name = llvm-libtool-darwin
 parent = Tools
-required_libraries = Object Support
+required_libraries = BinaryFormat Object Support

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

 //===-- llvm-libtool-darwin.cpp - a tool for creating libraries -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // A utility for creating static and dynamic libraries for Darwin.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
static Error verifyMachOObject(const NewArchiveMember &Member) {
   if (!isa<object::MachOObjectFile>(**ObjOrErr))
     return createStringError(std::errc::invalid_argument,
                              "'%s': format not supported",
                              Member.MemberName.data());
 
   return Error::success();
 }
 
-static Error addMember(std::vector<NewArchiveMember> &Members,
-                       StringRef FileName) {
+static Error addChildMember(std::vector<NewArchiveMember> &Members,
+                            const object::Archive::Child &M) {
   Expected<NewArchiveMember> NMOrErr =
-      NewArchiveMember::getFile(FileName, /*Deterministic=*/true);
+      NewArchiveMember::getOldMember(M, /*Deterministic=*/true);
+  if (!NMOrErr)
+    return NMOrErr.takeError();
+
+  // Verify that Member is a Mach-O object file.
+  if (Error E = verifyMachOObject(*NMOrErr))
+    return E;
 
+  Members.push_back(std::move(*NMOrErr));
+  return Error::success();
+}
+
+static Error
+addMember(std::vector<NewArchiveMember> &Members, StringRef FileName,
+          std::vector<std::unique_ptr<MemoryBuffer>> &ArchiveBuffers) {
+  Expected<NewArchiveMember> NMOrErr =
+      NewArchiveMember::getFile(FileName, /*Deterministic=*/true);
   if (!NMOrErr)
     return createFileError(FileName, NMOrErr.takeError());
 
   // For regular archives, use the basename of the object path for the member
   // name.
   NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
 
+  // Flatten archives.
+  if (identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
+    Expected<std::unique_ptr<Archive>> LibOrErr =
+        object::Archive::create(NMOrErr->Buf->getMemBufferRef());
+    if (!LibOrErr)
+      return createFileError(FileName, LibOrErr.takeError());
+    object::Archive &Lib = **LibOrErr;
+
+    Error Err = Error::success();
+    for (const object::Archive::Child &Child : Lib.children(Err))
+      if (Error E = addChildMember(Members, Child))
+        return createFileError(FileName, std::move(E));
+    if (Err)
+      return createFileError(FileName, std::move(Err));
+
+    // Update vector ArchiveBuffers with the MemoryBuffers to transfer
+    // ownership.
+    ArchiveBuffers.push_back(std::move(NMOrErr->Buf));
+    return Error::success();
+  }
+
   // Verify that Member is a Mach-O object file.
   if (Error E = verifyMachOObject(*NMOrErr))
     return E;
 
   Members.push_back(std::move(*NMOrErr));
   return Error::success();
 }
 
 static Error createStaticLibrary() {
   std::vector<NewArchiveMember> NewMembers;
+  std::vector<std::unique_ptr<MemoryBuffer>> ArchiveBuffers;
   for (StringRef Member : InputFiles)
-    if (Error E = addMember(NewMembers, Member))
+    if (Error E = addMember(NewMembers, Member, ArchiveBuffers))
       return E;
 
   if (Error E = writeArchive(OutputFile, NewMembers,
                              /*WriteSymtab=*/true,
                              /*Kind=*/object::Archive::K_DARWIN,
                              /*Deterministic=*/true,
                              /*Thin=*/false))
     return E;