[PATCH] D156505: [AArch64][ELF] Support R_AARCH64_AUTH_ABS64 static relocation

Thu Aug 3 17:21:51 PDT 2023

kovdan01 added a comment.

@MaskRay Thanks for such a detailed review! I've uploaded a new "intermediate" version with all the fixes except enhancing tests (e.g. adding missing ones). Marked the corresponding comments as done, feel free to re-open them if further discussion is needed. Regarding tests enhancements - I'll upload the patch fixing that shortly.

================
Comment at: docs/PointerAuth.md:333
+[Authenticated Relocations](#authenticated-global-relocation) are represented
+using the ``ARM64_RELOC_AUTHENTICATED_POINTER`` relocation kind (with value
+``0xE100``).
----------------
MaskRay wrote:
> `ARM64_RELOC_AUTHENTICATED_POINTER` is from Mach-O, but the sentence is written in the context of ELF. You need extra words to mean that this resembles `ARM64_RELOC_AUTHENTICATED_POINTER` from Mach-O.
> `ARM64_RELOC_AUTHENTICATED_POINTER` is from Mach-O, but the sentence is written in the context of ELF. You need extra words to mean that this resembles `ARM64_RELOC_AUTHENTICATED_POINTER` from Mach-O.

The sentence was supposed to mention `R_AARCH64_AUTH_ABS64`, thanks for noticing this. I think we should not mention the MachO relocation in context of the patch: (a) the patch only adds support for the ELF reloc; (b) AFAIK, a different signing schema encoding is used for the MachO reloc. So, just replaced `ARM64_RELOC_AUTHENTICATED_POINTER` -> `R_AARCH64_AUTH_ABS64` as initially intended.

================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:1
+// RUN: llvm-mc -triple=arm64-linux < %s | \
+// RUN: FileCheck %s --check-prefix=ASM
----------------
MaskRay wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > Did you not use ` git format-patch -1 ` or `git diff` to create a diff?
> > > 
> > > This creates a directory under `llvm-project`, instead of using `llvm/test/`
> > llvm-mc doesn't need `< %s`. Just use `%s`
> The test name should omit `arm64-` prefix, which is redundant for a file under `MC/AArch64`.
> Did you not use  git format-patch -1 or git diff to create a diff?
> 
> This creates a directory under llvm-project, instead of using llvm/test/

Thanks for bringing attention to this, fixed. It's quite strange though since I usually use `git show HEAD -U999999 > mypatch.patch` (as described here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface) which works fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156505