[llvm-objcopy] Accept MachO formats in commad-line parsing

[seiya]

Summary

This patch adds some MachO-specific fields to MachineInfo to
accept MachO formats in -B/-O options.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.cpp

272

Does Mach-O not support the other CPU types?

llvm/tools/llvm-objcopy/CopyConfig.h

52–53

I wonder if adding another constructor overload might make more sense for usage in the TargetMap table. What do you think?

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

306–307

The others appear to be:

./objcopy --info | grep ^mach-o | sort -u
mach-o-arm
mach-o-arm64
mach-o-be
mach-o-fat
mach-o-i386
mach-o-le
mach-o-x86-64

I don't think we need to add them all in this patch, but it'd be nice to see at least one more to make sure the test isn't too rigid. The rest can then be added in a trivial followup patch. (Or you can add them all now).

341–344

-freebsd seems to be an ELF-specific thing -- this should not be consumed if the target does not start with "elf"

llvm/tools/llvm-objcopy/CopyConfig.h

52–63

I don't think this constructor is actually used anymore

65–70

Might be good to extract these to separate types, then initialized like:

{"x86-64",
     {{ELF::EM_X86_64}, {MachO::CPU_TYPE_X86_64, MachO::CPU_SUBTYPE_X86_64_ALL},
     true, true}

to more clearly separate elf vs mach-o fields (and I imagine we could have COFF fields too)

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

271

It looks like GNU objcopy doesn't even accept -B x86-64, I wonder if we should consider dropping it. The arch used for that case is actually i386:x86-64.

272

It seems it accepts ~any CPU type (-I binary -B alpha -O mach-o-x86-64 works), though the effects don't seem to be visible (the CpuType/CpuSubType are always X86-64/CPU_SUBTYPE_X86_64_ALL regardless of the -B option).

[seiya]

Addressed review comments.

[seiya]

llvm/tools/llvm-objcopy/CopyConfig.cpp

271

I didn't noticed that. Personally, x86-64 looks intuitive to me but I think it should be removed for the compatibility with GNU objcopy. I'll write a patch for it when I have time.

272

Mach-O supports several CPU types (BinaryFormat/MachO.h). As @rupprecht described, GNU objcopy seems to ignore -B option and use CPUType/CPUSubType specified by -O option.

306–307

Added mach-o-arm64. I'll update the test in D66407 for it.

llvm/tools/llvm-objcopy/CopyConfig.h

52–53

Sounds good idea. Added overloads to MachineInfo.

[jhenderson]

Test cases for the new code?

llvm/tools/llvm-objcopy/CopyConfig.cpp

271

I don't think we should drop it. I agree that x86-64 is more intuitive, so if people are in a constrained environment with only the LLVM tools available to them, it's more useful. It's good to add stuff beyond what GNU supports for this reason, if there's a use case.

[seiya]

In D66449#1646539, @jhenderson wrote:

Test cases for the new code?

I forgot to add NFC in the title. This patch does not introduce new features thus we don't need new tests for it I think.

llvm/tools/llvm-objcopy/CopyConfig.cpp

271

It's good to add stuff beyond what GNU supports for this reason, if there's a use case.

Makes much sense.

[seiya]

In D66449#1648463, @seiya wrote:

In D66449#1646539, @jhenderson wrote:

Test cases for the new code?

I forgot to add NFC in the title. This patch does not introduce new features thus we don't need new tests for it I think.

Ah, I got it wrong. Tests for newly added code will be added in D66407.

[jhenderson]

LGTM.

llvm/tools/llvm-objcopy/CopyConfig.h

 #define LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/BinaryFormat/MachO.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Regex.h"
 // Necessary for llvm::DebugCompressionType::None
 #include "llvm/Target/TargetOptions.h"
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 
 enum class FileFormat {
   Unspecified,
   ELF,
+  MachO,
   Binary,
   IHex,
 };
 
+struct ELFMachineInfo {
+  uint16_t EMachine;
+  uint8_t OSABI;
+};
+
+struct MachOMachineInfo {
+  MachO::CPUType MachOCPUType;
+  uint32_t MachOCPUSubType;
+};
+
 // This type keeps track of the machine info for various architectures. This
-// lets us map architecture names to ELF types and the e_machine value of the
-// ELF file.
+// lets us map architecture names to format-specific values.
 struct MachineInfo {
+  // For ELF targets.
   MachineInfo(uint16_t EM, uint8_t ABI, bool Is64, bool IsLittle)
-      : EMachine(EM), OSABI(ABI), Is64Bit(Is64), IsLittleEndian(IsLittle) {}
-  // Alternative constructor that defaults to NONE for OSABI.
-  MachineInfo(uint16_t EM, bool Is64, bool IsLittle)
-      : MachineInfo(EM, ELF::ELFOSABI_NONE, Is64, IsLittle) {}
+      : ELF({EM, ABI}), MachO({MachO::CPU_TYPE_ANY, 0}), Is64Bit(Is64),
+        IsLittleEndian(IsLittle) {}
+  // For MachO targets.
+  MachineInfo(MachO::CPUType MachOCPUType, uint32_t MachOCPUSubType, bool Is64,
+              bool IsLittle)
+      : ELF({0, 0}), MachO({MachOCPUType, MachOCPUSubType}), Is64Bit(Is64),
+        IsLittleEndian(IsLittle) {}
+  MachineInfo(ELFMachineInfo ELF, MachOMachineInfo MachO, bool Is64,
+              bool IsLittle)
+      : ELF(ELF), MachO(MachO), Is64Bit(Is64), IsLittleEndian(IsLittle) {}
   // Default constructor for unset fields.
-  MachineInfo() : MachineInfo(0, 0, false, false) {}
-  uint16_t EMachine;
-  uint8_t OSABI;
+  MachineInfo()
+      : ELF({0, 0}), MachO({MachO::CPU_TYPE_ANY, 0}), Is64Bit(false),
+        IsLittleEndian(false) {}
+
+  ELFMachineInfo ELF;
+  MachOMachineInfo MachO;
+  // Common values.
   bool Is64Bit;
   bool IsLittleEndian;
 };
 
 // Flags set by --set-section-flags or --rename-section. Interpretation of these
 // is format-specific and not all flags are meaningful for all object file
 // formats. This is a bitmask; many section flags may be set.
 enum SectionFlag {

llvm/tools/llvm-objcopy/CopyConfig.cpp

 //===- CopyConfig.cpp -----------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "CopyConfig.h"
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/BinaryFormat/MachO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/JamCRC.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
if (!UnsupportedFlags.empty())
     return createStringError(errc::invalid_argument,
                              "unsupported flag%s for --add-symbol: '%s'",
                              UnsupportedFlags.size() > 1 ? "s" : "",
                              join(UnsupportedFlags, "', '").c_str());
   return SI;
 }
 
 static const StringMap<MachineInfo> ArchMap{
-    // Name, {EMachine, 64bit, LittleEndian}
-    {"aarch64", {ELF::EM_AARCH64, true, true}},
-    {"arm", {ELF::EM_ARM, false, true}},
-    {"i386", {ELF::EM_386, false, true}},
-    {"i386:x86-64", {ELF::EM_X86_64, true, true}},
-    {"mips", {ELF::EM_MIPS, false, false}},
-    {"powerpc:common64", {ELF::EM_PPC64, true, true}},
-    {"riscv:rv32", {ELF::EM_RISCV, false, true}},
-    {"riscv:rv64", {ELF::EM_RISCV, true, true}},
-    {"sparc", {ELF::EM_SPARC, false, false}},
-    {"sparcel", {ELF::EM_SPARC, false, true}},
-    {"x86-64", {ELF::EM_X86_64, true, true}},
+    // Name, {{EMachine, OSABI}, {MachOCPUType, MachOCPUSubType},
+    //         64bit, LittleEndian}
+    {"aarch64",
+     {{ELF::EM_AARCH64, 0},
+      {MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL},
+      true,
+      true}},
+    {"arm", {{ELF::EM_ARM, 0}, {MachO::CPU_TYPE_ANY, 0}, false, true}},
+    {"i386", {{ELF::EM_386, 0}, {MachO::CPU_TYPE_ANY, 0}, false, true}},
+    {"i386:x86-64",
+     {{ELF::EM_X86_64, 0}, {MachO::CPU_TYPE_ANY, 0}, true, true}},
+    {"mips", {{ELF::EM_MIPS, 0}, {MachO::CPU_TYPE_ANY, 0}, false, false}},
+    {"powerpc:common64",
+     {{ELF::EM_PPC64, 0}, {MachO::CPU_TYPE_ANY, 0}, true, true}},
+    {"riscv:rv32", {{ELF::EM_RISCV, 0}, {MachO::CPU_TYPE_ANY, 0}, false, true}},
+    {"riscv:rv64", {{ELF::EM_RISCV, 0}, {MachO::CPU_TYPE_ANY, 0}, true, true}},
+    {"sparc", {{ELF::EM_SPARC, 0}, {MachO::CPU_TYPE_ANY, 0}, false, false}},
+    {"sparcel", {{ELF::EM_SPARC, 0}, {MachO::CPU_TYPE_ANY, 0}, false, true}},
+    {"x86-64",
+     {{ELF::EM_X86_64, 0},
+      {MachO::CPU_TYPE_X86_64, MachO::CPU_SUBTYPE_X86_64_ALL},
+      true,
+      true}},
 };
 
 static Expected<const MachineInfo &> getMachineInfo(StringRef Arch) {
   auto Iter = ArchMap.find(Arch);
   if (Iter == std::end(ArchMap))
     return createStringError(errc::invalid_argument,
                              "invalid architecture: '%s'", Arch.str().c_str());
   return Iter->getValue();
 }
 
 struct TargetInfo {
   FileFormat Format;
   MachineInfo Machine;
 };
 
 // FIXME: consolidate with the bfd parsing used by lld.
 static const StringMap<MachineInfo> TargetMap{
-    // Name, {EMachine, 64bit, LittleEndian}
+    // ELF targets: Name, {EMachine, OSABI, 64bit, LittleEndian}
+    // Mach-O targets: Name, {CPUType, CPUSubType, 64bit, LittleEndian}
     // x86
-    {"elf32-i386", {ELF::EM_386, false, true}},
-    {"elf32-x86-64", {ELF::EM_X86_64, false, true}},
-    {"elf64-x86-64", {ELF::EM_X86_64, true, true}},
+    {"elf32-i386", {ELF::EM_386, 0, false, true}},
+    {"elf32-x86-64", {ELF::EM_X86_64, 0, false, true}},
+    {"elf64-x86-64", {ELF::EM_X86_64, 0, true, true}},
+    {"mach-o-x86-64",
+     {MachO::CPU_TYPE_X86_64, MachO::CPU_SUBTYPE_X86_64_ALL, true, true}},
+    {"mach-o-arm64",
+     {MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL, true, true}},
     // Intel MCU
-    {"elf32-iamcu", {ELF::EM_IAMCU, false, true}},
+    {"elf32-iamcu", {ELF::EM_IAMCU, 0, false, true}},
     // ARM
-    {"elf32-littlearm", {ELF::EM_ARM, false, true}},
+    {"elf32-littlearm", {ELF::EM_ARM, 0, false, true}},
     // ARM AArch64
-    {"elf64-aarch64", {ELF::EM_AARCH64, true, true}},
-    {"elf64-littleaarch64", {ELF::EM_AARCH64, true, true}},
+    {"elf64-aarch64", {ELF::EM_AARCH64, 0, true, true}},
+    {"elf64-littleaarch64", {ELF::EM_AARCH64, 0, true, true}},
     // RISC-V
-    {"elf32-littleriscv", {ELF::EM_RISCV, false, true}},
-    {"elf64-littleriscv", {ELF::EM_RISCV, true, true}},
+    {"elf32-littleriscv", {ELF::EM_RISCV, 0, false, true}},
+    {"elf64-littleriscv", {ELF::EM_RISCV, 0, true, true}},
     // PowerPC
-    {"elf32-powerpc", {ELF::EM_PPC, false, false}},
-    {"elf32-powerpcle", {ELF::EM_PPC, false, true}},
-    {"elf64-powerpc", {ELF::EM_PPC64, true, false}},
-    {"elf64-powerpcle", {ELF::EM_PPC64, true, true}},
+    {"elf32-powerpc", {ELF::EM_PPC, 0, false, false}},
+    {"elf32-powerpcle", {ELF::EM_PPC, 0, false, true}},
+    {"elf64-powerpc", {ELF::EM_PPC64, 0, true, false}},
+    {"elf64-powerpcle", {ELF::EM_PPC64, 0, true, true}},
     // MIPS
-    {"elf32-bigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-ntradbigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-ntradlittlemips", {ELF::EM_MIPS, false, true}},
-    {"elf32-tradbigmips", {ELF::EM_MIPS, false, false}},
-    {"elf32-tradlittlemips", {ELF::EM_MIPS, false, true}},
-    {"elf64-tradbigmips", {ELF::EM_MIPS, true, false}},
-    {"elf64-tradlittlemips", {ELF::EM_MIPS, true, true}},
+    {"elf32-bigmips", {ELF::EM_MIPS, 0, false, false}},
+    {"elf32-ntradbigmips", {ELF::EM_MIPS, 0, false, false}},
+    {"elf32-ntradlittlemips", {ELF::EM_MIPS, 0, false, true}},
+    {"elf32-tradbigmips", {ELF::EM_MIPS, 0, false, false}},
+    {"elf32-tradlittlemips", {ELF::EM_MIPS, 0, false, true}},
+    {"elf64-tradbigmips", {ELF::EM_MIPS, 0, true, false}},
+    {"elf64-tradlittlemips", {ELF::EM_MIPS, 0, true, true}},
     // SPARC
-    {"elf32-sparc", {ELF::EM_SPARC, false, false}},
-    {"elf32-sparcel", {ELF::EM_SPARC, false, true}},
+    {"elf32-sparc", {ELF::EM_SPARC, 0, false, false}},
+    {"elf32-sparcel", {ELF::EM_SPARC, 0, false, true}},
 };
 
 static Expected<TargetInfo>
 getOutputTargetInfoByTargetName(StringRef TargetName) {
   StringRef OriginalTargetName = TargetName;
-  bool IsFreeBSD = TargetName.consume_back("-freebsd");
+  bool IsFreeBSD = false;
+  if (TargetName.startswith("elf"))
+    IsFreeBSD = TargetName.consume_back("-freebsd");
+
   auto Iter = TargetMap.find(TargetName);
   if (Iter == std::end(TargetMap))
     return createStringError(errc::invalid_argument,
                              "invalid output format: '%s'",
                              OriginalTargetName.str().c_str());
   MachineInfo MI = Iter->getValue();
   if (IsFreeBSD)
-    MI.OSABI = ELF::ELFOSABI_FREEBSD;
+    MI.ELF.OSABI = ELF::ELFOSABI_FREEBSD;
 
   FileFormat Format;
   if (TargetName.startswith("elf"))
     Format = FileFormat::ELF;
+  else if (TargetName.startswith("mach-o"))
+    Format = FileFormat::MachO;
   else
     // This should never happen because `TargetName` is valid (it certainly
     // exists in the TargetMap).
     llvm_unreachable("unknown target prefix");
 
   return {TargetInfo{Format, MI}};
 }
 

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static Error splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
   auto DWOFile = Reader.create();
   auto OnlyKeepDWOPred = [&DWOFile](const SectionBase &Sec) {
     return onlyKeepDWOPred(*DWOFile, Sec);
   };
   if (Error E = DWOFile->removeSections(Config.AllowBrokenLinks, 
                                         OnlyKeepDWOPred))
     return E;
   if (Config.OutputArch) {
-    DWOFile->Machine = Config.OutputArch.getValue().EMachine;
-    DWOFile->OSABI = Config.OutputArch.getValue().OSABI;
+    DWOFile->Machine = Config.OutputArch.getValue().ELF.EMachine;
+    DWOFile->OSABI = Config.OutputArch.getValue().ELF.OSABI;
   }
   FileBuffer FB(File);
   auto Writer = createWriter(Config, *DWOFile, FB, OutputElfType);
   if (Error E = Writer->finalize())
     return E;
   return Writer->write();
 }
 
static Error handleArgs(const CopyConfig &Config, Object &Obj,
                         const Reader &Reader, ElfType OutputElfType) {
 
   if (!Config.SplitDWO.empty())
     if (Error E =
             splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType))
       return E;
 
   if (Config.OutputArch) {
-    Obj.Machine = Config.OutputArch.getValue().EMachine;
-    Obj.OSABI = Config.OutputArch.getValue().OSABI;
+    Obj.Machine = Config.OutputArch.getValue().ELF.EMachine;
+    Obj.OSABI = Config.OutputArch.getValue().ELF.OSABI;
   }
 
   // It is important to remove the sections first. For example, we want to
   // remove the relocation sections before removing the symbols. That allows
   // us to avoid reporting the inappropriate errors about removing symbols
   // named in relocations.
   if (Error E = replaceAndRemoveSections(Config, Obj))
     return E;

llvm/tools/llvm-objcopy/ELF/Object.cpp

template <class ELFT> void ELFBuilder<ELFT>::build() {
   readProgramHeaders(HeadersFile);
 }
 
 Writer::~Writer() {}
 
 Reader::~Reader() {}
 
 std::unique_ptr<Object> BinaryReader::create() const {
-  return BinaryELFBuilder(MInfo.EMachine, MemBuf).build();
+  return BinaryELFBuilder(MInfo.ELF.EMachine, MemBuf).build();
 }
 
 Expected<std::vector<IHexRecord>> IHexReader::parse() const {
   SmallVector<StringRef, 16> Lines;
   std::vector<IHexRecord> Records;
   bool HasSections = false;
 
   MemBuf->getBuffer().split(Lines, '\n');

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

static Error executeObjcopyOnRawBinary(const CopyConfig &Config,
   case FileFormat::ELF:
   // FIXME: Currently, we call elf::executeObjcopyOnRawBinary even if the
   // output format is binary/ihex or it's not given. This behavior differs from
   // GNU objcopy. See https://bugs.llvm.org/show_bug.cgi?id=42171 for details.
   case FileFormat::Binary:
   case FileFormat::IHex:
   case FileFormat::Unspecified:
     return elf::executeObjcopyOnRawBinary(Config, In, Out);
+  case FileFormat::MachO:
+    return createStringError(object_error::invalid_file_type,
+                             "unsupported output format");
   }
 
   llvm_unreachable("unsupported output format");
 }
 
 /// The function executeObjcopyOnBinary does the dispatch based on the format
 /// of the input binary (ELF, MachO or COFF).
 static Error executeObjcopyOnBinary(const CopyConfig &Config,