[llvm-lipo] Implement -archs

[anushabasana]

Summary

Displays the architecture names of an input file.
Unknown architectures are represented by unknown(cputype,cpusubtype).

Test Plan: check-llvm

[compnerd]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

2

Can you avoid the temporary using:

yaml2obj %s | llvm-lip - -archs | FileCheck %s

llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test

1

Similar

3

Nit: double space after "as"

llvm/test/tools/llvm-lipo/archs-universal-binary.test

1

Similar

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

56

This is confusing. Where is this used?

157

I don't think that this should be unreachable - if the action is unspecified, that can occur normally in the way that the user invokes the tool. Perhaps this should be a proper error?

218

Why not just make the parameter an OwningBinary<Binary> rather than the ArrayRef and then assert that a single entry is provided?

229

I don't think that this is necessarily unreachable - an invalid binary being given to the tool could hit this. This should be a proper error.

[smeenai]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

4

This should be worded better.

7

Nit: add a newline above this.

llvm/test/tools/llvm-lipo/archs-macho-binary.test

11–12

This one isn't specific to -archs; it should happen when combining any two flags.

There's an existing help-message.test. Let's rename that to something like command-line.test and move this check to there.

llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test

3

Comment should be worded better.

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

56

Line 64 below references LIPO_##PREFIX, and for an option group the prefix is nullptr. You should add a comment.

124–137

At this point, we should have a single arg inside ActionArgs, so we can just dispatch based on that instead of rechecking InputArgs. Something like

switch (ActionArgs[0]->getOption().getID()) {
case LIPO_verify_arch:
  ...
  break;
case LIPO_archs:
  ...
  break;
default:
  llvm_unreachable("unexpected action argument");
}

157

Lines 123 through 133 should ensure we have a single action argument of the expected type. My switch statement suggestion above should make that structure clearer.

218

Good idea. We'd probably also wanna take the parameter by const reference?

229

readInputBinaries above should have ensured we have a binary in the correct format.

[alexander-shaposhnikov]

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

157

llvm_unreachable is for internal logical errors, here if the specified command line arguments are incorrect we should make use of reportError, the tool should not crash.

[alexander-shaposhnikov]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

2

i think lipo doesn't support this (i.e. cat a.o | lipo - -archs wouldn't work), if we need this feature, we can add a proper support in llvm-lipo in the future, but i think it's a bit beyond the scope of the current patch.

[alexander-shaposhnikov]

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

229

i think here llvm_unreachable is appropriate since it should have been checked earlier, so here it's just a defensive assert (rather than do the same check and error reporting in multiple places)

[alexander-shaposhnikov]

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

218

not sure it really matters, the thing is that in general lipo can have multiple input files,
different actions expect different number of input files, thus since in main() we dispatch handling of particular commands to different subroutines i think it's clear / consistent enough. I'd prefer to not to make that "top-level" code less readable, although it's simple either way so i don't insist.

[alexander-shaposhnikov]

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

229

or convert it to assert

[alexander-shaposhnikov]

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

218

discussed with @compnerd and we agreed on this, might be worth adding a comment in the code (in main) that the details/specifics of handling particular options (i.e. the number of expected input files) are inside the corresponding subroutines.

[anushabasana]

Address review comments

[smeenai]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

2

Seems like It Just Works™. I guess the command line parsing just handles it?

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

157

I actually think this would be an internal logic error:

• If the user hasn't specified exactly one action argument, we'll have errored out already.
• The only action arguments we currently support are -verify_arch and -archs.
• Therefore, if our option isn't one of them, something is pretty wrong with our program logic.

I'm okay with using reportError for this though.

208

Nit: add a period at the end.

218

What did you have in mind for the comment? I feel like with the way the switch is structured right now it's pretty obvious that all details of handling a particular option are handled inside the function for it, and having a comment saying that seems unnecessary.

229

I think llvm_unreachable is better here, since the assert equivalent would be an assert(0) anyway.

[alexander-shaposhnikov]

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

157

imo it's not. llvm_unreachable wouldn't be the right choice here for the following reason: for example, if a user has misspelled a flag or option the tool should not crash with assert/llvm_unreachable, it should exit normally (report error and exit with non-zero code)

[alexander-shaposhnikov]

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

218

I didn't / I don't have a strong opinion regarding comments. Saleem wanted one.

[smeenai]

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

157

I agree, but if the user had misspelled a flag or option, we would have also handled it already (we check for unknown arguments explicitly above).

Basically, in order to have gotten to this switch statement, we should have performed all our input validation already. If we're ending up with an unknown option here, that means something is wrong with our input validation logic.

[smeenai]

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

157

Basically, under which circumstances could the default case be reached? I believe that with our current code we should never be able to reach it.

[alexander-shaposhnikov]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

2

it works with llvm-lipo because in https://llvm.org/doxygen/Binary_8cpp_source.html#l00092
MemoryBuffer::getFileOrSTDIN does the thing https://llvm.org/doxygen/MemoryBuffer_8cpp_source.html#l00143

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

157

depending on how good our validation will be in the future, this invariant ("unreachable") might be preserved, if one day this invariant gets violated - okay, the tool will start crashing on some incorrectly specified flags. So this was more like a general consideration what the tool should do if a flag or a combination of flags has been specified incorrectly, in particular, it should not crash.

[alexander-shaposhnikov]

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

2

the only downside which i see - if smb decides to grab the shell command and replace llvm-lipo with lipo it will stop working

[smeenai]

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

157

Sounds good, I can live with that.

[anushabasana]

Small changes from review comments

[anushabasana]

Ping

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

218

what sort of comment did you have in mind?

[alexander-shaposhnikov]

I think this looks okay / relatively small change anyway, i don't want to block it anymore, if there are any extra comments I think we can always either revert the diff or address them post-commit.

[mtrent]

I did not download or run this code, but I believe it does not correctly divine the Arch flag from a given Mach-O binary. This is straight-forward to fix.

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

209

The problem here is that there are Mach-O Arch types that are identifiable by cputype and cpusubtype, such as "x86_64h" and "armv7k". The general Triple API does not seem to support this; so Mach-O tools get this from the MachOObjectFile or from the MachOObjectFile::getArchTriple.

Since the code calling printArchFromCPUType has a MachOObjectFile (Obj), I would simply pass that MachOObjectFile in, ask it for its Triple (Obj->getArch()), and then handle an unknown triple. But you could remove the dependency on a MachOObjectFile by replacing your call to static MachOObjectFile::getArch() with a call to static MachOObjectFile::getArchTriple(): given the cputype and cpusubtype it will return the ArchFlag for you. But that's kind of backwards ...

224

See my note above; if it were me I'd pass Obj in here, and maybe rename the "printArchFromCPUType" to "printArchOrUnknown".

[alexander-shaposhnikov]

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

209

I missed this, thanks!
I think we need a test for this as well

[anushabasana]

Address review comments

[mtrent]

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

207

Ugh. I made a note to fix this trailing space in a future release. Sorry for the trouble, past and future.

[anushabasana]

Added Expected<> error check

llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test

+# RUN: yaml2obj %s > %t
+# Tests that the output for an unknown architecture is the same as cctools lipo
+# RUN: llvm-lipo %t -archs | FileCheck %s
+# CHECK: unknown(151,3)
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACE
+  cputype:         0x00000097
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           0
+  sizeofcmds:      0
+  flags:           0x00002000
+...

llvm/test/tools/llvm-lipo/archs-macho-binary.test

+# RUN: yaml2obj %s > %t
+
+# RUN: llvm-lipo %t -archs | FileCheck --check-prefix=ARCHS %s
+# RUN: llvm-lipo %t --archs | FileCheck --check-prefix=ARCHS %s
+# ARCHS: i386
+
+# RUN: not llvm-lipo %t %t -archs 2>&1 | FileCheck --check-prefix=MULTIPLE_INPUT_OBJ %s
+# MULTIPLE_INPUT_OBJ: archs expects a single input file
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACE
+  cputype:         0x00000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           0
+  sizeofcmds:      0
+  flags:           0x00002000
+...

llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test

 # RUN: yaml2obj %s > %t
-
-# RUN: llvm-lipo %t -verify_arch i386
-# RUN: llvm-lipo %t -verify_arch i386 x86_64
-
-# RUN: not llvm-lipo %t -verify_arch aarch64
-# RUN: not llvm-lipo %t -verify_arch aarch64 i386 
+# Tests that the output for an unknown architecture is the same as cctools lipo
+# RUN: llvm-lipo %t -archs 2>&1 | FileCheck %s
+# CHECK: i386 unknown(16777367,3)
 
 --- !fat-mach-o
-FatHeader:       
+FatHeader:
   magic:           0xCAFEBABE
   nfat_arch:       2
-FatArchs:        
+FatArchs:
   - cputype:         0x00000007
     cpusubtype:      0x00000003
     offset:          0x0000000000001000
     size:            28
     align:           12
-  - cputype:         0x01000007
+  - cputype:         0x01000097
     cpusubtype:      0x00000003
     offset:          0x0000000000002000
     size:            32
     align:           12
-Slices:          
+Slices:
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACE
       cputype:         0x00000007
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACF
-      cputype:         0x01000007
+      cputype:         0x01000097
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
       reserved:        0x00000000
 ...

llvm/test/tools/llvm-lipo/archs-universal-binary.test

 # RUN: yaml2obj %s > %t
-
-# RUN: llvm-lipo %t -verify_arch i386
-# RUN: llvm-lipo %t -verify_arch i386 x86_64
-
-# RUN: not llvm-lipo %t -verify_arch aarch64
-# RUN: not llvm-lipo %t -verify_arch aarch64 i386 
+# RUN: llvm-lipo %t -archs 2>&1 | FileCheck %s
+# CHECK: i386 x86_64
 
 --- !fat-mach-o
-FatHeader:       
+FatHeader:
   magic:           0xCAFEBABE
   nfat_arch:       2
-FatArchs:        
+FatArchs:
   - cputype:         0x00000007
     cpusubtype:      0x00000003
     offset:          0x0000000000001000
     size:            28
     align:           12
   - cputype:         0x01000007
     cpusubtype:      0x00000003
     offset:          0x0000000000002000
     size:            32
     align:           12
-Slices:          
+Slices:
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACE
       cputype:         0x00000007
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACF
       cputype:         0x01000007
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
       reserved:        0x00000000
 ...

llvm/test/tools/llvm-lipo/command-line.test

 # RUN: llvm-lipo -h | FileCheck --check-prefix=LIPO-USAGE %s
 # RUN: llvm-lipo --help | FileCheck --check-prefix=LIPO-USAGE %s
 
 # RUN: llvm-lipo -version | FileCheck --check-prefix=LIPO-VERSION %s
 # RUN: llvm-lipo --version | FileCheck --check-prefix=LIPO-VERSION %s
 
 # RUN: not llvm-lipo 2>&1 | FileCheck --check-prefix=LIPO-USAGE %s
 # RUN: not llvm-lipo -abcabc 2>&1 | FileCheck --check-prefix=LIPO-UNKNOWN-ARG %s
 # RUN: not llvm-lipo --abcabc 2>&1 | FileCheck --check-prefix=LIPO-UNKNOWN-ARG %s
 
+# RUN: not llvm-lipo %t -archs -verify_arch i386 2>&1 | FileCheck --check-prefix=MULTIPLE_FLAGS %s
+# MULTIPLE_FLAGS: only one of the following actions can be specified: -archs -verify_arch
+
 # LIPO-USAGE:    USAGE: llvm-lipo
 # LIPO-UNKNOWN-ARG:    unknown argument '{{-+}}abcabc'
 # LIPO-VERSION: {{ version }}

llvm/test/tools/llvm-lipo/verify-arch-universal-binary.test

 # RUN: yaml2obj %s > %t
 
 # RUN: llvm-lipo %t -verify_arch i386
 # RUN: llvm-lipo %t -verify_arch i386 x86_64
 
 # RUN: not llvm-lipo %t -verify_arch aarch64
-# RUN: not llvm-lipo %t -verify_arch aarch64 i386 
+# RUN: yaml2obj %s | not llvm-lipo - -verify_arch aarch64 i386
+# lipo does not support this (i.e. yaml2obj %s | not lipo -), included to test function with llvm-lipo
+# use the temporary %t when checking tests with lipo
 
 --- !fat-mach-o
-FatHeader:       
+FatHeader:
   magic:           0xCAFEBABE
   nfat_arch:       2
-FatArchs:        
+FatArchs:
   - cputype:         0x00000007
     cpusubtype:      0x00000003
     offset:          0x0000000000001000
     size:            28
     align:           12
   - cputype:         0x01000007
     cpusubtype:      0x00000003
     offset:          0x0000000000002000
     size:            32
     align:           12
-Slices:          
+Slices:
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACE
       cputype:         0x00000007
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
   - !mach-o
-    FileHeader:      
+    FileHeader:
       magic:           0xFEEDFACF
       cputype:         0x01000007
       cpusubtype:      0x00000003
       filetype:        0x00000001
       ncmds:           0
       sizeofcmds:      0
       flags:           0x00002000
       reserved:        0x00000000
 ...

llvm/tools/llvm-lipo/LipoOpts.td

 include "llvm/Option/OptParser.td"
 
 def help : Flag<["-", "--"], "help">;
 def h : Flag<["-"], "h">, Alias<help>;
 
 def version : Flag<["-", "--"], "version">,
               HelpText<"Print the version and exit.">;
 
-def verify_arch : Option<["-", "--"], "verify_arch", KIND_REMAINING_ARGS>,
-                  HelpText<"Verify that the specified arch_types are present in the input file">;
+def action_group : OptionGroup<"action group">;
+
+def verify_arch
+    : 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">;

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

enum LipoID {
   LIPO_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   LIPO_##ID,
 #include "LipoOpts.inc"
 #undef OPTION
 };
 
+// LipoInfoTable below references LIPO_##PREFIX. OptionGroup has prefix nullptr.
+const char *const *LIPO_nullptr = nullptr;
 #define PREFIX(NAME, VALUE) const char *const LIPO_##NAME[] = VALUE;
 #include "LipoOpts.inc"
 #undef PREFIX
 
 static const opt::OptTable::Info LipoInfoTable[] = {
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   {LIPO_##PREFIX, NAME,      HELPTEXT,                                         \
    METAVAR,       LIPO_##ID, opt::Option::KIND##Class,                         \
    PARAM,         FLAGS,     LIPO_##GROUP,                                     \
    LIPO_##ALIAS,  ALIASARGS, VALUES},
 #include "LipoOpts.inc"
 #undef OPTION
 };
 
 class LipoOptTable : public opt::OptTable {
 public:
   LipoOptTable() : OptTable(LipoInfoTable) {}
 };
 
+enum class LipoAction {
+  PrintArchs,
+  VerifyArch,
+};
+
 struct Config {
   SmallVector<std::string, 1> InputFiles;
   SmallVector<std::string, 1> VerifyArchList;
+  LipoAction ActionToPerform;
 };
 
 } // end namespace
 
 static Config parseLipoOptions(ArrayRef<const char *> ArgsArr) {
   Config C;
   LipoOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
static Config parseLipoOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(LIPO_UNKNOWN))
     reportError("unknown argument '" + Arg->getAsString(InputArgs) + "'");
 
   for (auto Arg : InputArgs.filtered(LIPO_INPUT))
     C.InputFiles.push_back(Arg->getValue());
   if (C.InputFiles.empty())
     reportError("at least one input file should be specified");
 
-  if (InputArgs.hasArg(LIPO_verify_arch)) {
+  SmallVector<opt::Arg *, 1> ActionArgs(InputArgs.filtered(LIPO_action_group));
+  if (ActionArgs.empty())
+    reportError("at least one action should be specified");
+  if (ActionArgs.size() > 1) {
+    std::string Buf;
+    raw_string_ostream OS(Buf);
+    OS << "only one of the following actions can be specified:";
+    for (auto Arg : ActionArgs)
+      OS << " " << Arg->getSpelling();
+    reportError(OS.str());
+  }
+
+  switch (ActionArgs[0]->getOption().getID()) {
+  case LIPO_verify_arch:
     for (auto A : InputArgs.getAllArgValues(LIPO_verify_arch))
       C.VerifyArchList.push_back(A);
     if (C.VerifyArchList.empty())
       reportError(
           "verify_arch requires at least one architecture to be specified");
     if (C.InputFiles.size() > 1)
       reportError("verify_arch expects a single input file");
-  }
+    C.ActionToPerform = LipoAction::VerifyArch;
     return C;
+
+  case LIPO_archs:
+    if (C.InputFiles.size() > 1)
+      reportError("archs expects a single input file");
+    C.ActionToPerform = LipoAction::PrintArchs;
+    return C;
+
+  default:
+    reportError("llvm-lipo action unspecified");
+  }
 }
 
 static SmallVector<OwningBinary<Binary>, 1>
 readInputBinaries(ArrayRef<std::string> InputFiles) {
   SmallVector<OwningBinary<Binary>, 1> InputBinaries;
   for (StringRef InputFile : InputFiles) {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(InputFile);
     if (!BinaryOrErr)
       reportError(InputFile, BinaryOrErr.takeError());
     if (!isa<MachOObjectFile>(BinaryOrErr->getBinary()) &&
         !isa<MachOUniversalBinary>(BinaryOrErr->getBinary()))
       reportError("File " + InputFile + " has unsupported binary format");
     InputBinaries.push_back(std::move(*BinaryOrErr));
   }
   return InputBinaries;
 }
 
 LLVM_ATTRIBUTE_NORETURN
 static void verifyArch(ArrayRef<OwningBinary<Binary>> InputBinaries,
                        ArrayRef<std::string> VerifyArchList) {
-  assert(!InputBinaries.empty() &&
-         "The list of input binaries should be non-empty");
   assert(!VerifyArchList.empty() &&
          "The list of architectures should be non-empty");
   assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
 
   for (StringRef Arch : VerifyArchList)
     if (Triple(Arch).getArch() == Triple::ArchType::UnknownArch)
       reportError("Invalid architecture: " + Arch);
 
for (StringRef Arch : VerifyArchList)
       if (ObjectArch != Triple(Arch).getArch())
         exit(EXIT_FAILURE);
   } else {
     llvm_unreachable("Unexpected binary format");
   }
   exit(EXIT_SUCCESS);
 }
 
+static void printArchFromCPUType(uint32_t CPUType, uint32_t CPUSubType) {
+  // Prints trailing space and unknown in this format for compatibility with
+  // cctools lipo.
+  Triple::ArchType Arch = MachOObjectFile::getArch(CPUType);
+  if (Arch == Triple::ArchType::UnknownArch)
+    outs() << "unknown(" << CPUType << ","
+           << (CPUSubType & ~MachO::CPU_SUBTYPE_MASK) << ") ";
+  else
+    outs() << Triple::getArchTypeName(Arch).str() << " ";
+}
+
+LLVM_ATTRIBUTE_NORETURN
+static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
+
+  if (auto UO =
+          dyn_cast<MachOUniversalBinary>(InputBinaries.front().getBinary()))
+    for (const MachOUniversalBinary::ObjectForArch &Obj : UO->objects())
+      printArchFromCPUType(Obj.getCPUType(), Obj.getCPUSubType());
+  else if (auto O =
+               dyn_cast<MachOObjectFile>(InputBinaries.front().getBinary()))
+    printArchFromCPUType(O->getHeader().cputype, O->getHeader().cpusubtype);
+  else
+    llvm_unreachable("Unexpected binary format");
+
+  outs() << "\n";
+  exit(EXIT_SUCCESS);
+}
+
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   Config C = parseLipoOptions(makeArrayRef(argv + 1, argc));
   SmallVector<OwningBinary<Binary>, 1> InputBinaries =
       readInputBinaries(C.InputFiles);
-  if (!C.VerifyArchList.empty())
+
+  switch (C.ActionToPerform) {
+  case LipoAction::VerifyArch:
     verifyArch(InputBinaries, C.VerifyArchList);
+    break;
+  case LipoAction::PrintArchs:
+    printArchs(InputBinaries);
+    break;
+  }
   return EXIT_SUCCESS;
 }