[llvm-lipo] Implement -info

[anushabasana]

Summary

Prints architecture type of all input files.

Test Plan: check-llvm

[anushabasana]

Fix diff

[compnerd]

llvm/tools/llvm-lipo/llvm-lipo.cpp

270 ↗

(On Diff #209608)

OS is the naming convention for the output stream

286 ↗

(On Diff #209608)

I think it may be nicer to change L280-L286 to something like:

    OS << '\n';
    return;
  }

  auto *O = cast<MachOObjectFile>(Binary);
  OS << getArchString(*O) << " \n";
}

318 ↗

(On Diff #209608)

Similar to above, you can use continue in the universal case and unindent the else case (make the Binary->isMachO() an assertion, and drop the unreachable.

547 ↗

(On Diff #209608)

Is there validation being applied here? I think that you should add a test case for invalid input as well (that is, pass ELF files to lipo and ensure that we handle that gracefully (right now, it feels like we may just abort?).

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

547 ↗

(On Diff #209608)

if I'm not mistaken readInputBinaries takes care of it, otherwise this would be reimplemented N times where N is the number of supported actions. Having an extra test of -info would not hurt though.

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

270 ↗

(On Diff #209608)

+1

301 ↗

(On Diff #209608)

khm, the same effect (grouping universal/regular files) could be achieved a bit simpler, what would you say to the following (no need for std::string, raw_string_ostream, ThinOutputBuffer, ThinOutputStream, UniversalOutputBuffer, UniversalOutputStream, etc)

 
for (const auto &IB : InputBinaries)
    if (IB.getBinary()->isMachOUniversalBinary()) {
         outs() << "Architectures in the fat file: " << Binary->getFileName() 
                   << " are: ";
         printBinaryArchs(IB.getBinary(), outs());
    }
 for (const auto &IB : InputBinaries)
    if (IB.getBinary()->isMachO) {
         outs() << "Non-fat file: " << IB.getBinary()->getFileName()
                   << " is architecture: ";
         printBinaryArchs(IB.getBinary(), outs());
    }

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

cc: @mtrent

1. The wording (especially in the second case) appears to be very weird, maybe it's worth to change it and make it more human-readable ? (I suspect -info is not really widely used in scripts, so probably this incompatibility might not be a big issue).
2. Maybe we don't even need to group anything here (for the same reasons)

[anushabasana]

Address review comments

[anushabasana]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

I could remove the string and OS for Universal files and just pass in outs() to cut down there.
I originally had 2 for loops but I thought the multiple OS would be better so we would only have to do one pass over the input binaries.
I'm not sure what is more legible, I don't have a strong preference.

[compnerd]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

@alexshap - I think that I would rather that we go with stronger claims:

for (const auto &IB : InputBinaries) {
  Binary *B = IB.getBinary();
  assert(B->isMachO() && "expected MachO binary");
  outs() << "Non-fat file: " << B->getFileName() << " is architecture: ";
  printBinaryArchs(B, outs());
}

547 ↗

(On Diff #209608)

Ah, okay, I was trying to see where that was called, I guess I just failed to see it. Yes, a test would be good still.

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

I don't think that performance is really a concern here (and in any case IO + allocations are more expensive than two "for" loops are), so I'd prefer to make this code as simple as possible,
in particular, get rid of these intermediate strings and buffers (I'm referring to ThinOutputBuffer, ThinOS, etc)

[compnerd]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

It seemed that this was confusing for @alexshap, what I meant is that this could be written as:

for (const auto &IB : InputBinaries) {
  if (IB.getBinary()->isMachOUniversalBinary()) {
    ...
  }
}
for (const auto &IB : InputBinaries) {
  if (!IB.getBinary()->isMachOUniversalBinary()) {
    ...
  }
}

which makes it more explicit that the two loops explicitly go over everything just filtering on a specific condition.

[anushabasana]

Removed extra OS

[anushabasana]

llvm/tools/llvm-lipo/llvm-lipo.cpp

547 ↗

(On Diff #209608)

I added test info-invalid.test

[smeenai]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

I've definitely seen -info be used in some scripts, particularly since -archs is a more recent invention, I think. I think it's best if we match lipo output here.

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

301 ↗

(On Diff #209608)

ok, then let's leave it as is

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

283 ↗

(On Diff #209917)

OS << getArchString(*cast<MachOObjectFile>(Binary))

[alexander-shaposhnikov]

llvm/tools/llvm-lipo/llvm-lipo.cpp

495 ↗

(On Diff #209917)

in order to ...

[anushabasana]

Address review comments

[alexander-shaposhnikov]

LG

[alexander-shaposhnikov]

llvm/test/tools/llvm-lipo/info.test

7 ↗

(On Diff #209943)

CHECK
CHECK-NEXT
CHECK-NEXT
...

(to verify that the order is correct)

[anushabasana]

Test file uses CHECK-NEXT instead of CHECK

llvm/trunk/test/tools/llvm-lipo/info-invalid.test

+# RUN: yaml2obj %s > %t
+# RUN: not llvm-lipo %t -info 2>&1 | FileCheck %s
+# CHECK: has unsupported binary format
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
+  Entry:           0x0000000000001000
+Sections:
+  - Name:            .gnu.version_d
+    Type:            SHT_GNU_verdef
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000000230
+    Link:            .dynstr
+    AddressAlign:    0x0000000000000004
+    Info:            0x0000000000000003
+    Entries:
+      - Version:         1
+        Flags:           1
+        VersionNdx:      1
+        Hash:            123456789
+        Names:
+          - foo
+      - Version:         1
+        Flags:           2
+        VersionNdx:      2
+        Hash:            987654321
+        Names:
+          - VERSION_1
+          - VERSION_2
+DynamicSymbols:
+  - Name:    bar
+    Binding: STB_GLOBAL
+...

llvm/trunk/test/tools/llvm-lipo/info.test

+# RUN: yaml2obj %p/Inputs/i386-slice.yaml > %t-i386.o
+# RUN: yaml2obj %p/Inputs/x86_64-slice.yaml > %t-x86_64.o
+# RUN: yaml2obj %p/Inputs/i386-x86_64-universal.yaml > %t-universal.o
+
+# RUN: llvm-lipo %t-universal.o %t-i386.o %t-universal.o %t-x86_64.o -info | FileCheck %s
+# CHECK: Architectures in the fat file:
+# CHECK: i386 x86_64
+# CHECK-NEXT: Architectures in the fat file:
+# CHECK: i386 x86_64
+# CHECK-NEXT: Non-fat file:
+# CHECK:  is architecture: i386
+# CHECK-NEXT: Non-fat file:
+# CHECK:  is architecture: x86_64

llvm/trunk/tools/llvm-lipo/LipoOpts.td

: Option<["-", "--"], "verify_arch", KIND_REMAINING_ARGS>,
       Group<action_group>,
       HelpText<
           "Verify that the specified arch_types are present in the input file">;
 
 def archs : Option<["-", "--"], "archs", KIND_FLAG>,
             Group<action_group>,
             HelpText<"Display the arch_types present in the input file">;
 
+def info : Option<["-", "--"], "info", KIND_FLAG>,
+           Group<action_group>,
+           HelpText<"Display descriptions of each input file including "
+                    "filename and arch_types. Groups universal binaries "
+                    "together followed by thin files">;
+
 def thin : Option<["-", "--"], "thin", KIND_SEPARATE>,
            Group<action_group>,
            HelpText<"Create a thin output file of specified arch_type from the "
                     "fat input file. Requires -output option">;
 
 def create : Option<["-", "--"], "create", KIND_FLAG>,
              Group<action_group>,
              HelpText<"Create a universal binary output file from the input "
                       "files. Requires -output option">;
 
 def output : Option<["-", "--"], "output", KIND_SEPARATE>,
              HelpText<"Create output file with specified name">;
 def o : JoinedOrSeparate<["-"], "o">, Alias<output>;

llvm/trunk/tools/llvm-lipo/llvm-lipo.cpp

 
 class LipoOptTable : public opt::OptTable {
 public:
   LipoOptTable() : OptTable(LipoInfoTable) {}
 };
 
 enum class LipoAction {
   PrintArchs,
+  PrintInfo,
   VerifyArch,
   ThinArch,
   CreateUniversal,
 };
 
 struct Config {
   SmallVector<std::string, 1> InputFiles;
   SmallVector<std::string, 1> VerifyArchList;
case LIPO_verify_arch:
     return C;
 
   case LIPO_archs:
     if (C.InputFiles.size() > 1)
       reportError("archs expects a single input file");
     C.ActionToPerform = LipoAction::PrintArchs;
     return C;
 
+  case LIPO_info:
+    C.ActionToPerform = LipoAction::PrintInfo;
+    return C;
+
   case LIPO_thin:
     if (C.InputFiles.size() > 1)
       reportError("thin expects a single input file");
     C.ThinArchType = ActionArgs[0]->getValue();
     validateArchitectureName(C.ThinArchType);
     if (C.OutputFile.empty())
       reportError("thin expects a single output file");
 
static std::string getArchString(const MachOObjectFile &ObjectFile) {
   if (!ObjectArch.empty())
     return ObjectArch;
   return ("unknown(" + Twine(ObjectFile.getHeader().cputype) + "," +
           Twine(ObjectFile.getHeader().cpusubtype & ~MachO::CPU_SUBTYPE_MASK) +
           ")")
       .str();
 }
 
-LLVM_ATTRIBUTE_NORETURN
-static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+static void printBinaryArchs(const Binary *Binary, raw_ostream &OS) {
   // Prints trailing space for compatibility with cctools lipo.
-  assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
-  const Binary *InputBinary = InputBinaries.front().getBinary();
-  if (auto UO = dyn_cast<MachOUniversalBinary>(InputBinary)) {
+  if (auto UO = dyn_cast<MachOUniversalBinary>(Binary)) {
     for (const auto &O : UO->objects()) {
       Expected<std::unique_ptr<MachOObjectFile>> BinaryOrError =
           O.getAsObjectFile();
       if (!BinaryOrError)
-        reportError(InputBinary->getFileName(), BinaryOrError.takeError());
-      outs() << getArchString(*BinaryOrError.get().get()) << " ";
+        reportError(Binary->getFileName(), BinaryOrError.takeError());
+      OS << getArchString(*BinaryOrError.get().get()) << " ";
     }
-  } else if (auto O = dyn_cast<MachOObjectFile>(InputBinary)) {
-    outs() << getArchString(*O) << " ";
-  } else {
-    llvm_unreachable("Unexpected binary format");
-  }
+    OS << "\n";
+    return;
+  }
+  OS << getArchString(*cast<MachOObjectFile>(Binary)) << " \n";
 
-  outs() << "\n";
+LLVM_ATTRIBUTE_NORETURN
+static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
+  printBinaryArchs(InputBinaries.front().getBinary(), outs());
+  exit(EXIT_SUCCESS);
+}
+
+LLVM_ATTRIBUTE_NORETURN
+static void printInfo(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  // Group universal and thin files together for compatibility with cctools lipo
+  for (auto &IB : InputBinaries) {
+    const Binary *Binary = IB.getBinary();
+    if (Binary->isMachOUniversalBinary()) {
+      outs() << "Architectures in the fat file: " << Binary->getFileName()
+             << " are: ";
+      printBinaryArchs(Binary, outs());
+    }
+  }
+  for (auto &IB : InputBinaries) {
+    const Binary *Binary = IB.getBinary();
+    if (!Binary->isMachOUniversalBinary()) {
+      assert(Binary->isMachO() && "expected MachO binary");
+      outs() << "Non-fat file: " << Binary->getFileName()
+             << " is architecture: ";
+      printBinaryArchs(Binary, outs());
+    }
+  }
   exit(EXIT_SUCCESS);
 }
 
 LLVM_ATTRIBUTE_NORETURN
 static void extractSlice(ArrayRef<OwningBinary<Binary>> InputBinaries,
                          StringRef ThinArchType, StringRef OutputFileName) {
   assert(!ThinArchType.empty() && "The architecture type should be non-empty");
   assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
static void createUniversalBinary(SmallVectorImpl<Slice> &Slices,
   std::memcpy(OutFile->getBufferStart(), &FatHeader, sizeof(MachO::fat_header));
 
   for (size_t Index = 0, Size = Slices.size(); Index < Size; ++Index) {
     MemoryBufferRef BufferRef = Slices[Index].ObjectFile->getMemoryBufferRef();
     std::copy(BufferRef.getBufferStart(), BufferRef.getBufferEnd(),
               OutFile->getBufferStart() + FatArchList[Index].offset);
   }
 
-  // FatArchs written after Slices in order reduce the number of swaps for the
-  // LittleEndian case
+  // FatArchs written after Slices in order to reduce the number of swaps for
+  // the LittleEndian case
   if (sys::IsLittleEndianHost)
     for (MachO::fat_arch &FA : FatArchList)
       MachO::swapStruct(FA);
   std::memcpy(OutFile->getBufferStart() + sizeof(MachO::fat_header),
               FatArchList.begin(),
               sizeof(MachO::fat_arch) * FatArchList.size());
 
   if (Error E = OutFile->commit())
int main(int argc, char **argv) {
 
   switch (C.ActionToPerform) {
   case LipoAction::VerifyArch:
     verifyArch(InputBinaries, C.VerifyArchList);
     break;
   case LipoAction::PrintArchs:
     printArchs(InputBinaries);
     break;
+  case LipoAction::PrintInfo:
+    printInfo(InputBinaries);
+    break;
   case LipoAction::ThinArch:
     extractSlice(InputBinaries, C.ThinArchType, C.OutputFile);
     break;
   case LipoAction::CreateUniversal:
     createUniversalBinary(InputBinaries, C.OutputFile);
     break;
   }
   return EXIT_SUCCESS;
 }