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

[pguo]

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.

[pguo]

Fix commit message.

[pguo]

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.

[jhenderson]

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]

Inline the check.

[pguo]

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]

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

llvm/tools/llvm-objdump/MachODump.cpp

459

getArch().isAArch64()

[pguo]

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.

[jhenderson]

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.

[dsanders]

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]

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.

llvm/tools/llvm-objdump/MachODump.cpp

if (O->isRelocationScattered(RE)) {
     }
 
     Fmt << format("0x%x", Val);
     return;
   }
 
   StringRef S;
   bool isExtern = O->getPlainRelocationExternal(RE);
+  bool isArm64 =
+      O->getArch() == Triple::aarch64 || O->getArch() == Triple::aarch64_be;
   uint64_t Val = O->getPlainRelocationSymbolNum(RE);
 
-  if (O->getAnyRelocationType(RE) == MachO::ARM64_RELOC_ADDEND) {
+  if (isArm64 && O->getAnyRelocationType(RE) == MachO::ARM64_RELOC_ADDEND) {
     Fmt << format("0x%0" PRIx64, Val);
     return;
   }
 
   if (isExtern) {
     symbol_iterator SI = O->symbol_begin();
     advance(SI, Val);
     S = unwrapOrError(SI->getName(), FileName);
   } else {