[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

40

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

296–297

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).

329

-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

45–49

I don't think this constructor is actually used anymore

53–58

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.

296–297

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

llvm/tools/llvm-objcopy/CopyConfig.h

40

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

 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 
 enum class FileFormat {
   Unspecified,
   ELF,
+  MachO,
   Binary,
   IHex,
 };
 
 // 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 {
-  MachineInfo(uint16_t EM, uint8_t ABI, bool Is64, bool IsLittle)
-      : EMachine(EM), OSABI(ABI), Is64Bit(Is64), IsLittleEndian(IsLittle) {}
+  MachineInfo(uint16_t EM, uint8_t ABI, uint32_t MachOCPUType,
+              uint32_t MachOCPUSubType, bool Is64, bool IsLittle)
+      : EMachine(EM), OSABI(ABI), MachOCPUType(MachOCPUType),
+        MachOCPUSubType(MachOCPUSubType), 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) {}
+  MachineInfo(uint16_t EM, uint32_t MachOCPUType, uint32_t MachOSubCPUType,
+              bool Is64, bool IsLittle)
+      : MachineInfo(EM, ELF::ELFOSABI_NONE, MachOCPUType, MachOSubCPUType, Is64,
+                    IsLittle) {}
   // Default constructor for unset fields.
-  MachineInfo() : MachineInfo(0, 0, false, false) {}
+  MachineInfo() : MachineInfo(0, 0, 0, 0, false, false) {}
+
+  // ELF values.
   uint16_t EMachine;
   uint8_t OSABI;
+  // MachO values.
+  uint32_t MachOCPUType;
+  uint32_t MachOCPUSubType;
+  // 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, MachOCPUType, MachOCPUSubType, 64bit, LittleEndian}
+    {"aarch64", {ELF::EM_AARCH64, 0, 0, true, true}},
+    {"arm", {ELF::EM_ARM, 0, 0, false, true}},
+    {"i386", {ELF::EM_386, 0, 0, false, true}},
+    {"i386:x86-64", {ELF::EM_X86_64, 0, 0, true, true}},
+    {"mips", {ELF::EM_MIPS, 0, 0, false, false}},
+    {"powerpc:common64", {ELF::EM_PPC64, 0, 0, true, true}},
+    {"riscv:rv32", {ELF::EM_RISCV, 0, 0, false, true}},
+    {"riscv:rv64", {ELF::EM_RISCV, 0, 0, true, true}},
+    {"sparc", {ELF::EM_SPARC, 0, 0, false, false}},
+    {"sparcel", {ELF::EM_SPARC, 0, 0, false, true}},
+    {"x86-64",
+     {ELF::EM_X86_64, 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}
+    // Name, {EMachine, MachOCPUType, MachOCPUSubType, 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, 0, false, true}},
+    {"elf32-x86-64", {ELF::EM_X86_64, 0, 0, false, true}},
+    {"elf64-x86-64", {ELF::EM_X86_64, 0, 0, true, true}},
+    {"mach-o-x86-64",
+     {0, MachO::CPU_TYPE_X86_64, MachO::CPU_SUBTYPE_X86_64_ALL, true, true}},
     // Intel MCU
-    {"elf32-iamcu", {ELF::EM_IAMCU, false, true}},
+    {"elf32-iamcu", {ELF::EM_IAMCU, 0, 0, false, true}},
     // ARM
-    {"elf32-littlearm", {ELF::EM_ARM, false, true}},
+    {"elf32-littlearm", {ELF::EM_ARM, 0, 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, 0, true, true}},
+    {"elf64-littleaarch64", {ELF::EM_AARCH64, 0, 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, 0, false, true}},
+    {"elf64-littleriscv", {ELF::EM_RISCV, 0, 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, 0, false, false}},
+    {"elf32-powerpcle", {ELF::EM_PPC, 0, 0, false, true}},
+    {"elf64-powerpc", {ELF::EM_PPC64, 0, 0, true, false}},
+    {"elf64-powerpcle", {ELF::EM_PPC64, 0, 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, 0, false, false}},
+    {"elf32-ntradbigmips", {ELF::EM_MIPS, 0, 0, false, false}},
+    {"elf32-ntradlittlemips", {ELF::EM_MIPS, 0, 0, false, true}},
+    {"elf32-tradbigmips", {ELF::EM_MIPS, 0, 0, false, false}},
+    {"elf32-tradlittlemips", {ELF::EM_MIPS, 0, 0, false, true}},
+    {"elf64-tradbigmips", {ELF::EM_MIPS, 0, 0, true, false}},
+    {"elf64-tradlittlemips", {ELF::EM_MIPS, 0, 0, true, true}},
     // SPARC
-    {"elf32-sparc", {ELF::EM_SPARC, false, false}},
-    {"elf32-sparcel", {ELF::EM_SPARC, false, true}},
+    {"elf32-sparc", {ELF::EM_SPARC, 0, 0, false, false}},
+    {"elf32-sparcel", {ELF::EM_SPARC, 0, 0, false, true}},
 };
 
 static Expected<TargetInfo>
 getOutputTargetInfoByTargetName(StringRef TargetName) {
   StringRef OriginalTargetName = TargetName;
   bool 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;
 
   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}};
 }