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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:7522
+  if (Parser.getTok().isNot(AsmToken::Identifier))
+    return TokError("expected key name in @AUTH expression");
+
----------------
Many of the error messages are untested.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:7539
+  if (!isUInt<16>(Discriminator))
+    return TokError("too wide integer discriminator '" + itostr(Discriminator) +
+                    "'in @AUTH expression");
----------------
itostr requires including `StringExtras.h`. That said, you can use `Twine(Discriminator)` here.

Also, this error should be covered by a test.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:7555
+  EndLoc = Parser.getTok().getEndLoc();
+  if (parseToken(AsmToken::RParen,
+                 "expected ')' at the end of @AUTH expression"))
----------------
The error message is not tested.

I think a simple `"expected ')'"` is sufficient. The integrated assembler marks the column that an error occurs, so it's obvious that this message applies to `@AUTH`.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:7561
+                                  UseAddressDiversity, Ctx);
+
+  return false;
----------------
nit: delete blank line


================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp:326
+    if (TheTriple.isOSBinFormatELF()) {
+      AArch64MCExpr::VariantKind RefKind =
+          static_cast<AArch64MCExpr::VariantKind>(Target.getRefKind());
----------------
Nit


================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:1
+// RUN: llvm-mc -triple=arm64-linux < %s | \
+// RUN: FileCheck %s --check-prefix=ASM
----------------
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/`


================
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:
> 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`


================
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:
> > 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`.


================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:4
+
+// RUN: llvm-mc -triple=arm64-linux -filetype=obj < %s | \
+// RUN: llvm-readobj --expand-relocs --sections --section-relocations --section-data - | \
----------------
Prefer aarch64 for Linux target triples: https://nickdesaulniers.github.io/blog/2023/03/10/disambiguating-arm/

`aarch64` (generic ELF) is better than `*-linux` as the former applies to other ELF OSes like *BSD.


================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:5
+// RUN: llvm-mc -triple=arm64-linux -filetype=obj < %s | \
+// RUN: llvm-readobj --expand-relocs --sections --section-relocations --section-data - | \
+// RUN: FileCheck %s --check-prefix=RELOC
----------------
`llvm-readobj` is quite verbose. Tabular output from `llvm-readelf -S -r` may look better. Use `-x sectionname` for content.


================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:112
+// ASM:          .xword _g1 at AUTH(ib,0)
+.quad _g1 at AUTH(ib,0)
+.quad 0
----------------
While here, consider making some `_g*` symbols defined or give them a local binding to add some variance.


================
Comment at: test/MC/AArch64/arm64-ptrauth-elf-reloc.s:114
+.quad 0
+
+// ASM:          .xword _g2 at AUTH(da,5,addr)
----------------
A test involving more modifiers like `a at PLT@AUTH(ib,0)` may be interesting to add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156505