[PATCH] D93045: [ELF] AArch64: Handle DT_AARCH64_VARIANT_PCS

[Peter Smith via Phabricator via llvm-commits]

psmith added a comment.

Thanks for working on this. It looks like it implements the ABI correctly (see PR for links to the specification and GCC/Glibc details).

A few small nits, but otherwise looks good to me.



================
Comment at: lld/ELF/SyntheticSections.cpp:1441
+      if (in.relaPlt->aarch64VariantPcs)
+        addInt(DT_AARCH64_VARIANT_PCS, 0);
+      LLVM_FALLTHROUGH;
----------------
Does this change depend on another one adding DT_AARCH64_VARIANT_PCS? I think these are defined in /llvm/include/BinaryFormat/DynamicTags.def


================
Comment at: lld/ELF/SyntheticSections.cpp:2192
       eSym->st_other |= sym->stOther & 0xe0;
+    else if (config->emachine == EM_AARCH64)
+      eSym->st_other |= sym->stOther & STO_AARCH64_VARIANT_PCS;
----------------
May be worth a comment like the OpenPOWER ABI to explain that st_other is being used by the AArch64 ABI for the variant PCS.


================
Comment at: lld/test/ELF/aarch64-variant_pcs-not-required.s:50
+
+# To avoid create multiple permutations, make all defined symbols branch
+# to all symbols
----------------
Nit: "create" should be "creating"


================
Comment at: lld/test/ELF/aarch64-variant_pcs.s:10
+
+## Check if variant_pcs are exported and st_other it set correctly.
+# PCSSYM: Symbol table '.dynsym'
----------------
nit: "Check if variant_pcs is exported and st_other is set correctly."


================
Comment at: lld/test/ELF/aarch64-variant_pcs.s:18
+# PCSSYM:        0 IFUNC  GLOBAL DEFAULT [VARIANT_PCS]   7 pcs_ifunc_global_def
+# PCSSYM: Symbol table '.symtab'
+# PCSSYM:        0 NOTYPE LOCAL  DEFAULT [VARIANT_PCS]   7 pcs_func_local
----------------
Maybe worth not including the static symbol table in the test; while there is nothing wrong with keeping the variant pcs flag in the static symbol table, I don't think it is required for an executable or shared library.


================
Comment at: lld/test/ELF/aarch64-variant_pcs.s:68
+
+# To avoid create multiple permutations, make all defined symbols branch
+# to all symbols
----------------
Nit: "create" should be "creating"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93045