objdump: Better handling of Mach-O universal binaries

[kastiglione]

Summary

With Mach-O, there is a flag requirement discrepancy between working with
universal binaries and thin binaries. Many flags that don't require the -macho
flag (for example -private-headers and -disassemble) fail to work on
universal binaries unless -macho is given. When this happens, the error
message is unhelpful, stating:

The file was not recognized as a valid object file.

Which can lead to confusion.

This change allows generic flags to be used on universal binaries with and
without the -macho flag. This means flags that can be used for thin files can
be used consistently with fat files too.

To do this, the universal binary support within ParseInputMachO() is extracted
into a new function. This new function is called directly from DumpInput()
when the input binary is universal. Additionally the -arch flag validation in
ParseInputMachO() was extracted to be reused.

[compnerd]

tools/llvm-objdump/MachODump.cpp

2014 ↗

(On Diff #153253)

Yes, I know you just moved the code, but please either remove or keep braces on both sides.

[kastiglione]

Make if/else bracing consistent

[kastiglione]

Oops, fix last diff update

[compnerd]

tools/llvm-objdump/MachODump.cpp

1961 ↗

(On Diff #154353)

This leaves the old behavior in place, but I think it would be nicer if the arch flag was validated earlier.

[kastiglione]

tools/llvm-objdump/MachODump.cpp

1961 ↗

(On Diff #154353)

Did you have something specific in mind? By the caller, before calling this function? Or something else…

llvm/trunk/test/tools/llvm-objdump/X86/macho-private-headers.test

 // RUN: llvm-objdump -p %p/Inputs/dylibSubClient.macho-x86_64 \
 // RUN:     | FileCheck %s -check-prefix=SUB_CLI
 // RUN: llvm-objdump -p %p/Inputs/dylibRoutines.macho-x86_64 \
 // RUN:     | FileCheck %s -check-prefix=ROUTINE
 // RUN: llvm-objdump -p %p/Inputs/exeThread.macho-x86_64 \
 // RUN:     | FileCheck %s -check-prefix=THREAD
 // RUN: llvm-objdump -macho -p -arch i386 %p/Inputs/macho-universal.x86_64.i386 \
 // RUN:     | FileCheck %s -check-prefix=FATi386
+// RUN: llvm-objdump -p -arch i386 %p/Inputs/macho-universal.x86_64.i386 \
+// RUN:     | FileCheck %s -check-prefix=FATi386
 // RUN: llvm-objdump -p -non-verbose %p/Inputs/hello.obj.macho-x86_64 \
 // RUN:     | FileCheck %s -check-prefix=NON_VERBOSE
 // RUN: llvm-objdump -p %p/Inputs/codesig.macho-x86_64 \
 // RUN:     | FileCheck %s -check-prefix=CODESIG
 // RUN: llvm-objdump -p %p/Inputs/note.macho-x86 \
 // RUN:     | FileCheck %s -check-prefix=NOTE
 
 CHECK: Mach header

llvm/trunk/test/tools/llvm-objdump/X86/macho-universal-x86_64.i386.test

 RUN: llvm-objdump %p/Inputs/macho-universal.x86_64.i386 -d -m -no-show-raw-insn -full-leading-addr -print-imm-hex -arch all \
 RUN: | FileCheck %s -check-prefix UEXE-all
+RUN: llvm-objdump %p/Inputs/macho-universal.x86_64.i386 -d -no-show-raw-insn -full-leading-addr -print-imm-hex -arch all \
+RUN: | FileCheck %s -check-prefix UEXE-all
 RUN: llvm-objdump %p/Inputs/macho-universal-archive.x86_64.i386 -d -m -no-show-raw-insn -full-leading-addr -print-imm-hex -arch i386 \
 RUN: | FileCheck %s -check-prefix UArchive-i386
 RUN: llvm-objdump %p/Inputs/macho-universal.x86_64.i386 -universal-headers -m \
 RUN: | FileCheck %s -check-prefix FAT
 RUN: llvm-objdump %p/Inputs/macho-universal.x86_64.i386 -universal-headers -m \
 RUN: -non-verbose | FileCheck %s -check-prefix NON-VERBOSE
 RUN: llvm-objdump %p/Inputs/macho-universal64.x86_64.i386 -universal-headers \
 RUN: -m | FileCheck %s -check-prefix FAT-64

llvm/trunk/tools/llvm-objdump/MachODump.cpp

static void printArchiveHeaders(StringRef Filename, Archive *A, bool verbose,
   ;
   for (const auto &C : A->children(Err, false))
     printArchiveChild(Filename, C, verbose, print_offset, ArchitectureName);
 
   if (Err)
     report_error(StringRef(), Filename, std::move(Err), ArchitectureName);
 }
 
-// ParseInputMachO() parses the named Mach-O file in Filename and handles the
-// -arch flags selecting just those slices as specified by them and also parses
-// archive files.  Then for each individual Mach-O file ProcessMachO() is
-// called to process the file based on the command line options.
-void llvm::ParseInputMachO(StringRef Filename) {
+static bool ValidateArchFlags() {
   // Check for -arch all and verifiy the -arch flags are valid.
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {
     if (ArchFlags[i] == "all") {
       ArchAll = true;
     } else {
       if (!MachOObjectFile::isValidArch(ArchFlags[i])) {
         errs() << "llvm-objdump: Unknown architecture named '" + ArchFlags[i] +
                       "'for the -arch option\n";
-        return;
+        return false;
       }
     }
   }
+  return true;
+}
+
+// ParseInputMachO() parses the named Mach-O file in Filename and handles the
+// -arch flags selecting just those slices as specified by them and also parses
+// archive files.  Then for each individual Mach-O file ProcessMachO() is
+// called to process the file based on the command line options.
+void llvm::ParseInputMachO(StringRef Filename) {
+  if (!ValidateArchFlags())
+    return;
 
   // Attempt to open the binary.
   Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(Filename);
   if (!BinaryOrErr) {
     if (auto E = isNotObjectErrorInvalidFileType(BinaryOrErr.takeError()))
       report_error(Filename, std::move(E));
     else
       outs() << Filename << ": is not an object file\n";
for (auto &C : A->children(Err)) {
           return;
         ProcessMachO(Filename, O, O->getFileName());
       }
     }
     if (Err)
       report_error(Filename, std::move(Err));
     return;
   }
-  if (UniversalHeaders) {
-    if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(&Bin))
-      printMachOUniversalHeaders(UB, !NonVerbose);
-  }
   if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(&Bin)) {
-    // If we have a list of architecture flags specified dump only those.
-    if (!ArchAll && ArchFlags.size() != 0) {
-      // Look for a slice in the universal binary that matches each ArchFlag.
-      bool ArchFound;
-      for (unsigned i = 0; i < ArchFlags.size(); ++i) {
-        ArchFound = false;
-        for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
-                                                   E = UB->end_objects();
-             I != E; ++I) {
-          if (ArchFlags[i] == I->getArchFlagName()) {
-            ArchFound = true;
-            Expected<std::unique_ptr<ObjectFile>> ObjOrErr =
-                I->getAsObjectFile();
-            std::string ArchitectureName = "";
-            if (ArchFlags.size() > 1)
-              ArchitectureName = I->getArchFlagName();
-            if (ObjOrErr) {
-              ObjectFile &O = *ObjOrErr.get();
-              if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&O))
-                ProcessMachO(Filename, MachOOF, "", ArchitectureName);
-            } else if (auto E = isNotObjectErrorInvalidFileType(
-                       ObjOrErr.takeError())) {
-              report_error(Filename, StringRef(), std::move(E),
-                           ArchitectureName);
-              continue;
-            } else if (Expected<std::unique_ptr<Archive>> AOrErr =
-                           I->getAsArchive()) {
-              std::unique_ptr<Archive> &A = *AOrErr;
-              outs() << "Archive : " << Filename;
-              if (!ArchitectureName.empty())
-                outs() << " (architecture " << ArchitectureName << ")";
-              outs() << "\n";
-              if (ArchiveHeaders)
-                printArchiveHeaders(Filename, A.get(), !NonVerbose,
-                                    ArchiveMemberOffsets, ArchitectureName);
-              Error Err = Error::success();
-              for (auto &C : A->children(Err)) {
-                Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
-                if (!ChildOrErr) {
-                  if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-                    report_error(Filename, C, std::move(E), ArchitectureName);
-                  continue;
-                }
-                if (MachOObjectFile *O =
-                        dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
-                  ProcessMachO(Filename, O, O->getFileName(), ArchitectureName);
-              }
-              if (Err)
-                report_error(Filename, std::move(Err));
-            } else {
-              consumeError(AOrErr.takeError());
-              error("Mach-O universal file: " + Filename + " for " +
-                    "architecture " + StringRef(I->getArchFlagName()) +
-                    " is not a Mach-O file or an archive file");
-            }
-          }
-        }
-        if (!ArchFound) {
-          errs() << "llvm-objdump: file: " + Filename + " does not contain "
-                 << "architecture: " + ArchFlags[i] + "\n";
-          return;
-        }
-      }
-      return;
-    }
-    // No architecture flags were specified so if this contains a slice that
-    // matches the host architecture dump only that.
-    if (!ArchAll) {
-      for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
-                                                 E = UB->end_objects();
-           I != E; ++I) {
-        if (MachOObjectFile::getHostArch().getArchName() ==
-            I->getArchFlagName()) {
-          Expected<std::unique_ptr<ObjectFile>> ObjOrErr = I->getAsObjectFile();
-          std::string ArchiveName;
-          ArchiveName.clear();
-          if (ObjOrErr) {
-            ObjectFile &O = *ObjOrErr.get();
-            if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&O))
-              ProcessMachO(Filename, MachOOF);
-          } else if (auto E = isNotObjectErrorInvalidFileType(
-                     ObjOrErr.takeError())) {
-            report_error(Filename, std::move(E));
-            continue;
-          } else if (Expected<std::unique_ptr<Archive>> AOrErr =
-                         I->getAsArchive()) {
-            std::unique_ptr<Archive> &A = *AOrErr;
-            outs() << "Archive : " << Filename << "\n";
-            if (ArchiveHeaders)
-              printArchiveHeaders(Filename, A.get(), !NonVerbose,
-                                  ArchiveMemberOffsets);
-            Error Err = Error::success();
-            for (auto &C : A->children(Err)) {
-              Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
-              if (!ChildOrErr) {
-                if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-                  report_error(Filename, C, std::move(E));
-                continue;
-              }
-              if (MachOObjectFile *O =
-                      dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
-                ProcessMachO(Filename, O, O->getFileName());
-            }
-            if (Err)
-              report_error(Filename, std::move(Err));
-          } else {
-            consumeError(AOrErr.takeError());
-            error("Mach-O universal file: " + Filename + " for architecture " +
-                  StringRef(I->getArchFlagName()) +
-                  " is not a Mach-O file or an archive file");
-          }
-          return;
-        }
-      }
-    }
-    // Either all architectures have been specified or none have been specified
-    // and this does not contain the host architecture so dump all the slices.
-    bool moreThanOneArch = UB->getNumberOfObjects() > 1;
-    for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
-                                               E = UB->end_objects();
-         I != E; ++I) {
-      Expected<std::unique_ptr<ObjectFile>> ObjOrErr = I->getAsObjectFile();
-      std::string ArchitectureName = "";
-      if (moreThanOneArch)
-        ArchitectureName = I->getArchFlagName();
-      if (ObjOrErr) {
-        ObjectFile &Obj = *ObjOrErr.get();
-        if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&Obj))
-          ProcessMachO(Filename, MachOOF, "", ArchitectureName);
-      } else if (auto E = isNotObjectErrorInvalidFileType(
-                 ObjOrErr.takeError())) {
-        report_error(StringRef(), Filename, std::move(E), ArchitectureName);
-        continue;
-      } else if (Expected<std::unique_ptr<Archive>> AOrErr =
-                   I->getAsArchive()) {
-        std::unique_ptr<Archive> &A = *AOrErr;
-        outs() << "Archive : " << Filename;
-        if (!ArchitectureName.empty())
-          outs() << " (architecture " << ArchitectureName << ")";
-        outs() << "\n";
-        if (ArchiveHeaders)
-          printArchiveHeaders(Filename, A.get(), !NonVerbose,
-                              ArchiveMemberOffsets, ArchitectureName);
-        Error Err = Error::success();
-        for (auto &C : A->children(Err)) {
-          Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
-          if (!ChildOrErr) {
-            if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-              report_error(Filename, C, std::move(E), ArchitectureName);
-            continue;
-          }
-          if (MachOObjectFile *O =
-                  dyn_cast<MachOObjectFile>(&*ChildOrErr.get())) {
-            if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(O))
-              ProcessMachO(Filename, MachOOF, MachOOF->getFileName(),
-                           ArchitectureName);
-          }
-        }
-        if (Err)
-          report_error(Filename, std::move(Err));
-      } else {
-        consumeError(AOrErr.takeError());
-        error("Mach-O universal file: " + Filename + " for architecture " +
-              StringRef(I->getArchFlagName()) +
-              " is not a Mach-O file or an archive file");
-      }
-    }
-    return;
-  }
-  if (ObjectFile *O = dyn_cast<ObjectFile>(&Bin)) {
-    if (!checkMachOAndArchFlags(O, Filename))
-      return;
-    if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&*O)) {
-      ProcessMachO(Filename, MachOOF);
-    } else
-      errs() << "llvm-objdump: '" << Filename << "': "
-             << "Object is not a Mach-O file type.\n";
-    return;
-  }
-  llvm_unreachable("Input object can't be invalid at this point");
+    ParseInputMachO(UB);
+    return;
+  }
+  if (ObjectFile *O = dyn_cast<ObjectFile>(&Bin)) {
+    if (!checkMachOAndArchFlags(O, Filename))
+      return;
+    if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&*O))
+      ProcessMachO(Filename, MachOOF);
+    else
+      errs() << "llvm-objdump: '" << Filename << "': "
+             << "Object is not a Mach-O file type.\n";
+    return;
+  }
+  llvm_unreachable("Input object can't be invalid at this point");
+}
+
+void llvm::ParseInputMachO(MachOUniversalBinary *UB) {
+  if (!ValidateArchFlags())
+    return;
+
+  auto Filename = UB->getFileName();
+
+  if (UniversalHeaders)
+    printMachOUniversalHeaders(UB, !NonVerbose);
+
 }
 
 // The block of info used by the Symbolizer call backs.
 struct DisassembleInfo {
   DisassembleInfo(MachOObjectFile *O, SymbolAddressMap *AddrMap,
                   std::vector<SectionRef> *Sections, bool verbose)
     : verbose(verbose), O(O), AddrMap(AddrMap), Sections(Sections) {}
   bool verbose;

llvm/trunk/tools/llvm-objdump/llvm-objdump.h

 
 namespace llvm {
 class StringRef;
 
 namespace object {
   class COFFObjectFile;
   class COFFImportFile;
   class MachOObjectFile;
+  class MachOUniversalBinary;
   class ObjectFile;
   class Archive;
   class RelocationRef;
 }
 
 extern cl::opt<std::string> TripleName;
 extern cl::opt<std::string> ArchName;
 extern cl::opt<std::string> MCPU;

 extern cl::opt<bool> UnwindInfo;
 extern cl::opt<bool> PrintImmHex;
 extern cl::opt<DIDumpType> DwarfDumpType;
 
 // Various helper functions.
 void error(std::error_code ec);
 bool RelocAddressLess(object::RelocationRef a, object::RelocationRef b);
 void ParseInputMachO(StringRef Filename);
+void ParseInputMachO(object::MachOUniversalBinary *UB);
 void printCOFFUnwindInfo(const object::COFFObjectFile* o);
 void printMachOUnwindInfo(const object::MachOObjectFile* o);
 void printMachOExportsTrie(const object::MachOObjectFile* o);
 void printMachORebaseTable(object::MachOObjectFile* o);
 void printMachOBindTable(object::MachOObjectFile* o);
 void printMachOLazyBindTable(object::MachOObjectFile* o);
 void printMachOWeakBindTable(object::MachOObjectFile* o);
 void printELFFileHeader(const object::ObjectFile *o);

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachO.h"
+#include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/Wasm.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
static void DumpInput(StringRef file) {
   if (!BinaryOrErr)
     report_error(file, BinaryOrErr.takeError());
   Binary &Binary = *BinaryOrErr.get().getBinary();
 
   if (Archive *a = dyn_cast<Archive>(&Binary))
     DumpArchive(a);
   else if (ObjectFile *o = dyn_cast<ObjectFile>(&Binary))
     DumpObject(o);
+  else if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(&Binary))
+    ParseInputMachO(UB);
   else
     report_error(file, object_error::invalid_file_type);
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   // Initialize targets and assembly printers/parsers.