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

[Seiya Nuta via Phabricator via llvm-commits]

seiya added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:271
+    {"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,
----------------
rupprecht wrote:
> 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`.
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.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:272
+    {"x86-64",
+     {ELF::EM_X86_64, MachO::CPU_TYPE_X86_64, MachO::CPU_SUBTYPE_X86_64_ALL,
+      true, true}},
----------------
rupprecht wrote:
> jhenderson wrote:
> > Does Mach-O not support the other CPU types?
> 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).
Mach-O supports several CPU types ([[ https://github.com/llvm/llvm-project/blob/522377494b3d7c4bfcaa9a632497231ac3c19143/llvm/include/llvm/BinaryFormat/MachO.h#L1414 | BinaryFormat/MachO.h ]]). As @rupprecht described, GNU objcopy seems to ignore `-B` option and use CPUType/CPUSubType specified by `-O` option.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:296-297
+    {"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
----------------
rupprecht wrote:
> 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).
Added `mach-o-arm64`. I'll update the test in D66407 for it.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:40
 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)
----------------
jhenderson wrote:
> I wonder if adding another constructor overload might make more sense for usage in the TargetMap table. What do you think?
Sounds good idea. Added overloads to MachineInfo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66449/new/

https://reviews.llvm.org/D66449