This is an archive of the discontinued LLVM Phabricator instance.

[objdump][macho] Check arch before formating reloc name as arm64 addend
ClosedPublic

Authored by pguo on Oct 8 2020, 9:52 PM.

Details

Summary

Before formating ARM64_RELOC_ADDEND relocation target name as a hex
number, the architecture need to be checked since other architectures
can define a different relocation type with the same integer as
ARM64_RELOC_ADDEND.

Diff Detail

Event Timeline

pguo created this revision.Oct 8 2020, 9:52 PM
pguo requested review of this revision.Oct 8 2020, 9:52 PM
pguo updated this revision to Diff 297122.

Fix commit message.

pguo edited the summary of this revision. (Show Details)Oct 8 2020, 9:58 PM
pguo added reviewers: dsanders, mstorsjo.
pguo added a comment.Oct 8 2020, 10:09 PM

I didn't add a test in the patch since for now, there only two macho relocation types using number 10: PPC_RELOC_HI16_SECTDIFF and ARM64_RELOC_ADDEND. And PPC macho object writer has been deleted in commit 3e851f4a688.

Harbormaster completed remote builds in B74528: Diff 297121.

This is outside my area of expertise to review, but regarding testing, you should use yaml2obj to create a Mach-O input with AARCH64 type (and also one with the big-endian version too), and then also one with a different machine type to show the opposite behaviour happens. There are plenty of examples of Mach-O yaml2obj inputs in various places. Take a look in existing tests to find them.

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

Alternatively, I'd just inline the check as it's only used in one place.

pguo updated this revision to Diff 297764.Oct 12 2020, 10:06 PM

Inline the check.

pguo added a comment.EditedOct 12 2020, 10:15 PM

For the macho, there already has a test case which is llvm/test/tools/llvm-objdump/MachO/AArch64/macho-reloc-addend.test. And to test this change, we need a macho with different architecture which using a different relocation type with enum value 10 (which is the enum value of MachO::ARM64_RELOC_ADDEND). The issue is for current llvm, only PPC defined a different relocation type with value 10, but the macho backend for PPC has been deleted from llvm. So currently we cannot make such a test in upstream llvm.
The logic in the patch is simple and clear, so probably it's fine without a test.

MaskRay accepted this revision.Oct 12 2020, 10:19 PM

(MachoDump.cpp needs more love. It is overly complicated and greatly under tested/maintenance)

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

getArch().isAArch64()

This revision is now accepted and ready to land.Oct 12 2020, 10:19 PM
pguo added inline comments.Oct 12 2020, 10:56 PM
llvm/tools/llvm-objdump/MachODump.cpp
459

The O->getArch() returns enum Triple::ArchType, not a Triple. Maybe we can refactor the Triple arch checking functions to something like (as a separate commit):

class Triple {
  static bool isAArch64(ArchType Arch) {
    return Arch == aarch64 || Arch == aarch64_be;
  }
  bool isAArch64() const {
    return Triple::isAArch64(getArch());
  }
}

Then we can reuse the Triple::isAArch64 here.

Okay, I see the issue, thanks for explaining. In ELF yaml2obj and llvm-readobj, you can explicitly specify unknown values for some fields using hex, I believe, rather than the enum name. Is that possible for relocations using the Mach-O versions? If not, looks good to me too.

Peng asked me to commit this off-list

Is that possible for relocations using the Mach-O versions?

I'll leave him to comment on that but FWIW I think it would be reasonable for it to error out on a relocation id that isn't known to a target. At best we'd be testing that an undefined relocation behaves a particular way

pguo added a comment.Oct 16 2020, 2:58 PM

I looked at the objdump macho unit tests. Looks like the yaml2obj is used only in a small portion of unit test, and majority are still binary inputs. For macho yaml input, it contains many magic numbers. So IMO it probably better to commit this simple change first, and improve objdump macho unit tests progressively.