This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Add constant CPU_TYPE_ARM64_V8
ClosedPublic

Authored by sameerarora101 on Jul 31 2020, 12:07 PM.

Details

Summary

Add support for constant MachO::CPU_SUBTYPE_ARM64_V8. This constant is
needed so as to match llvm-libtool-darwin's behavior to that of
cctools' libtool when -arch_only flag is passed in on command line.
Here is the diff of llvm-libtool-darwin adding arch_only option for
reference: https://reviews.llvm.org/D84770.

Diff Detail

Event Timeline

sameerarora101 requested review of this revision.Jul 31 2020, 12:07 PM

I'd probably add a simple test for llvm-readobj as well.

MaskRay added inline comments.Aug 2 2020, 9:46 PM
llvm/test/tools/llvm-objdump/MachO/AArch64/macho-arm64-v8.test
1 ↗(On Diff #282295)

Usually RUN and CHECK lines should use the same comment marker. Either you include the comment marker or leave the comment marker for both RUN and CHECK

In D85041#2188548, @alexshap wrote:

I'd probably add a simple test for llvm-readobj as well.

+1. Whilst you're at it. You might as well test the other types too in that enum (assuming they aren't already tested). You can use yaml2obj's -D to reuse the YAML for each input file.

llvm/test/tools/llvm-objdump/MachO/AArch64/Inputs/arm64_v8.macho.yaml
1 ↗(On Diff #282295)

As this input is only used in one place, you can put it inline with the test that uses it. You'll need to use '#' for the RUN/CHECKs in the test then.

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

Are there tests for these other cases too? If not, perhaps we should make the new test more generic, and test them all in one test?

sameerarora101 marked 2 inline comments as done.Aug 3 2020, 11:31 AM
sameerarora101 added inline comments.
llvm/tools/llvm-objdump/MachODump.cpp
2108

Under llvm-objdump, for CPU-TYPE CPU_TYPE_ARM64,

  • there is one test case for subtype CPU_SUBTYPE_ARM64E (macho-arm64e.test)
  • one test case (which I added above) for CPU_SUBTYPE_ARM64_V8 (macho-arm64-v8.test)
  • all remaining tests are for subtype CPU_SUBTYPE_ARM64_ALL

Add test for llvm-readobj

jhenderson added inline comments.Aug 4 2020, 12:47 AM
llvm/test/tools/llvm-objdump/MachO/AArch64/macho-arm64-v8.test
1 ↗(On Diff #282692)

Rather than add a test nearly identical to macho-arm64e.test, what do you think about renaming that test to macho-arm64-subtypes.test (or something similar) and merging the two (optionally with an explicit testing of the ALL subtype, but that's not so important since there's coverage elsewhere)

llvm/test/tools/llvm-readobj/MachO/file-headers-arm64.test
29–44

Rather than duplicating all these CHECKs just for the sake of one different one, you could use FileCheck's -D option or --check-prefixes option to share them.

Example using -D:

# RUN:   | FileCheck %s -DSUBTYPE="CPU_SUBTYPE_ARM64_ALL (0x0)" ...

#      CHECK:File: [[FILE]]
...
# CHECK-NEXT:  CpuSubType: [[SUBTYPE]]
...

# RUN:   | FileCheck %s -DSUBTYPE="CPU_SUBTYPE_ARM64_V8 (0x1)" ...
# RUN:   | FileCheck %s -DSUBTYPE="CPU_SUBTYPE_ARM64E (0x2)" ...

Using --check-prefixes:

# RUN:   | FileCheck %s --check-prefixes="COMMON,ALL" ...
...
# RUN:   | FileCheck %s --check-prefixes="COMMON,V8" ...
...
# RUN:   | FileCheck %s --check-prefixes="COMMON,E" ...

#      COMMON:File: [[FILE]]
...
# COMMON-NEXT:  CpuType: Arm64 (0x100000C)
#    ALL-NEXT:  CpuSubType: CPU_SUBTYPE_ARM64_ALL (0x0)
#     V8-NEXT:  CpuSubType: CPU_SUBTYPE_ARM64_V8 (0x1)
#      E-NEXT:  CpuSubType: CPU_SUBTYPE_ARM64E (0x2)
# COMMON-NEXT:  FileType: Relocatable (0x1)
...
llvm/tools/llvm-objdump/MachODump.cpp
2112–2115

This code-path doesn't seem to have any testing? The new objdump test only covers the other change in this file by my reading.

sameerarora101 marked 3 inline comments as done.Aug 4 2020, 8:21 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-objdump/MachO/AArch64/macho-arm64-v8.test
1 ↗(On Diff #282692)

Yeah, I agree. Updated it now, thanks!

llvm/tools/llvm-objdump/MachODump.cpp
2112–2115

True. Added a test for them while printing universal headers now (similar to universal-x86_64.i386.test) Thanks.

sameerarora101 marked 2 inline comments as done.

Update tests.

This revision is now accepted and ready to land.Aug 4 2020, 12:57 PM
smeenai accepted this revision.Aug 4 2020, 2:43 PM

LGTM, but please wait for @jhenderson too.

jhenderson accepted this revision.Aug 5 2020, 12:17 AM

LGTM, with optional nit.

llvm/test/tools/llvm-objdump/MachO/universal-arm64.test
7

Nit: as you only have one check prefix in this file, you can get rid of --check-prefix and just use the default CHECK

sameerarora101 marked an inline comment as done.Aug 5 2020, 7:26 AM

Remove --check-prefix=FAT.

llvm/test/tools/llvm-objdump/MachO/AArch64/macho-arm64-subtypes.test