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

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added inline comments.


================
Comment at: lld/ELF/SyntheticSections.cpp:1441
+      if (in.relaPlt->aarch64VariantPcs)
+        addInt(DT_AARCH64_VARIANT_PCS, 0);
+      LLVM_FALLTHROUGH;
----------------
psmith wrote:
> Does this change depend on another one adding DT_AARCH64_VARIANT_PCS? I think these are defined in /llvm/include/BinaryFormat/DynamicTags.def
Yes, I forgot to add on patch description that https://reviews.llvm.org/D93044 is a prerequisite. It adds both the DT_AARCH64 fields and the llvm-readelf VARIANT_PCS symbol tag.


================
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;
----------------
psmith wrote:
> 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.
Ack, I will add one.


================
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
----------------
psmith wrote:
> Nit: "create" should be "creating"
Ack.


================
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
----------------
psmith wrote:
> 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.
Right, I will add only the GLOBAL ones then.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93045