Page MenuHomePhabricator

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

Authored by seiya on Aug 19 2019, 3:21 PM.

Details

Summary

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

Event Timeline

seiya created this revision.Aug 19 2019, 3:21 PM
jhenderson added inline comments.Aug 20 2019, 2:15 AM
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 added inline comments.Aug 20 2019, 5:13 PM
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 added inline comments.Aug 21 2019, 1:22 PM
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 updated this revision to Diff 217087.Aug 26 2019, 1:22 AM
seiya marked 12 inline comments as done.

Addressed review comments.

seiya added inline comments.Aug 26 2019, 1:22 AM
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.

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 retitled this revision from [llvm-objcopy] Accept MachO formats in commad-line parsing to [llvm-objcopy] Accept MachO formats in commad-line parsing. NFC..Aug 27 2019, 8:31 PM
seiya marked an inline comment as done.

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 retitled this revision from [llvm-objcopy] Accept MachO formats in commad-line parsing. NFC. to [llvm-objcopy] Accept MachO formats in commad-line parsing.Aug 27 2019, 8:34 PM
seiya added a comment.Aug 27 2019, 8:38 PM

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.

This revision is now accepted and ready to land.Aug 28 2019, 2:45 AM
alexshap accepted this revision.Wed, Oct 2, 4:12 AM