[ELF] AArch64: Handle DT_AARCH64_VARIANT_PCS

[zatrazz]

Summary

As indicated by AArch64 ELF specification, symbols with st_other
marked with STO_AARCH64_VARIANT_PCS indicates a may follow a variant
procedure call standard with different register usage convention
(for instance SVE calls).

Static linkers must preserve the marking and propagate it to the dynamic
symbol table if any reference or definition of the symbol is marked with
STO_AARCH64_VARIANT_PCS, and add a DT_AARCH64_VARIANT_PCS dynamic tag if
there are R_<CLS>_JUMP_SLOT relocations that reference that symbols.

It fixes https://bugs.llvm.org/show_bug.cgi?id=48368.

[psmith]

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.

lld/ELF/SyntheticSections.cpp

1441

Does this change depend on another one adding DT_AARCH64_VARIANT_PCS? I think these are defined in /llvm/include/BinaryFormat/DynamicTags.def

2191

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.

lld/test/ELF/aarch64-variant_pcs-not-required.s

50 ↗

(On Diff #310916)

Nit: "create" should be "creating"

lld/test/ELF/aarch64-variant_pcs.s

11

nit: "Check if variant_pcs is exported and st_other is set correctly."

19

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.

69

Nit: "create" should be "creating"

[MaskRay]

st_other marked with STO_AARCH64_VARIANT_PCS indicates a may follow a variant

a -> it

if there are R_<CLS>_JUMP_SLOT relocations that reference that symbols.

Question: The ABI does use the <CLS> notation. Does it mean it can be AARCH32 in the future?

It fixes https://bugs.llvm.org/show_bug.cgi?id=48368.

I think "fixes" is used for bug fixes. This is a new feature.

binutils elfNN_aarch64_merge_symbol_attribute has an untested diagnostic _bfd_error_handler (_("unknown attribute for symbol %s': 0x%02x"), h->root.root.string, isym_sto);` (@nsz)
Is it useful to LLD?

lld/ELF/SyntheticSections.cpp

1440

Instead of introducing a member aarch64VariantPcs, would iterating in.relaPlt->relocs here make the AArch64 specific code more isolated?

lld/ELF/SyntheticSections.h

509 ↗

(On Diff #310916)

Prefer member initializer = false;

lld/test/ELF/aarch64-variant_pcs.s

13

{{0+}} is not useful.

(Even if you want to check this is not a canonical PLT entry (which is probably out of scope of this test), {{0+}} can match trailing zeros in a non-zero address 12340)

14

Use -NEXT. The FileCheck diagnostic will be more friendly if a newline is added or a line is removed.

15

Replace 7 with [[#]] to prevent updatess when section indexes change.

35

Optional: .global a, b, c can mark several symbols

[zatrazz]

lld/ELF/SyntheticSections.cpp

1441

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.

2191

Ack, I will add one.

lld/test/ELF/aarch64-variant_pcs-not-required.s

50 ↗

(On Diff #310916)

Ack.

lld/test/ELF/aarch64-variant_pcs.s

19

Right, I will add only the GLOBAL ones then.

69

Ack.

[zatrazz]

In D93045#2446599, @MaskRay wrote:

st_other marked with STO_AARCH64_VARIANT_PCS indicates a may follow a variant

a -> it

Ack.

if there are R_<CLS>_JUMP_SLOT relocations that reference that symbols.

Question: The ABI does use the <CLS> notation. Does it mean it can be AARCH32 in the future?

This is better described on section "5.7.3.1Relocation names and class" from the documentation and the another possible <CLS> would be AARCH64_P32 (basically the AArch64 ILP32 ABI that has been developed out of tree and is on beta status regarding ABI).

It fixes https://bugs.llvm.org/show_bug.cgi?id=48368.

I think "fixes" is used for bug fixes. This is a new feature.

'It implements' would be better then?

binutils elfNN_aarch64_merge_symbol_attribute has an untested diagnostic _bfd_error_handler (_("unknown attribute for symbol %s': 0x%02x"), h->root.root.string, isym_sto);` (@nsz)
Is it useful to LLD?

I think it might be useful to warn whether old lld is being used along objects built with newer ABI extensions. I can work on add a warning on a subsequent patch.

lld/test/ELF/aarch64-variant_pcs.s

13

Indeed, this is not really required. I will removed and check only if 'func_global_undef' is being exported.

14

Ack.

15

Ack.

35

Ack.

[MaskRay]

lld/test/ELF/aarch64-variant_pcs.s

19

I think testing .symtab is fine and probably favorable because otherwise local symbols cannot be tested. While their values are useful to ld.so, their values could be read by programs performing reflection and other binary manipulating programs.

[zatrazz]

lld/ELF/SyntheticSections.cpp

1440

I though about about, but I decided to add a new member since it might be costly to iterate over all relocations to check if it is a target->relPlt and if the symbols has the STO_AARCH64_VARIANT_PCS. I don't have a strong preference here in fact.

lld/ELF/SyntheticSections.h

509 ↗

(On Diff #310916)

Ack.

[zatrazz]

Updated patch based on previous comments.

[MaskRay]

lld/ELF/SyntheticSections.cpp

1440

How is costly to iterate in.relaPlt?

It actually performs less testing than config->emachine == EM_AARCH64 && reloc.type == target->pltRel which has an additional emachine test.

There is also a question whether in.relaIplt and R_AARCH64_IRELATIVE should be tested.

[zatrazz]

lld/ELF/SyntheticSections.cpp

1440

It depends of how many relocation are within in.relaPlt, but I think it should not penalize other architectures. I will move the test in the case of EM_AARCH64 only.

And we, adding solely R_AARCH64_JMP_SLOT to ifunc should generate the dynamic tag. I will fix it as well.

[zatrazz]

lld/ELF/SyntheticSections.cpp

1441

In fact the in.relaIplt is tricky because the ABI does not really state R_AARCH64_IRELATIVE should also add the dynamic tag. However I think it makes sense since it will act exactly as R_AARCH64_JUMP_SLOT once ifunc is resolved.

[zatrazz]

Updated patch based on previous comments. For in.relaIplt / R_AARCH64_IRELATIVE
I decided to not include since it seems to what binutils is done currently. I will raise with
them if this is the intended behaviour or if R_AARCH64_IRELATIVE should also create
the dynamic tag (and fix is accordingly on lld if it were the case).

[peter.smith]

In D93045#2449509, @zatrazz wrote:

Updated patch based on previous comments. For in.relaIplt / R_AARCH64_IRELATIVE
I decided to not include since it seems to what binutils is done currently. I will raise with
them if this is the intended behaviour or if R_AARCH64_IRELATIVE should also create
the dynamic tag (and fix is accordingly on lld if it were the case).

As I understand it, IRELATIVE relocations are not resolved lazily by the dynamic loader so there would not need to be a variant PCS flag in this particular case. So I think it is likely binutils is right here. No harm in checking though.

[MaskRay]

In D93045#2451478, @peter.smith wrote:

In D93045#2449509, @zatrazz wrote:

Updated patch based on previous comments. For in.relaIplt / R_AARCH64_IRELATIVE
I decided to not include since it seems to what binutils is done currently. I will raise with
them if this is the intended behaviour or if R_AARCH64_IRELATIVE should also create
the dynamic tag (and fix is accordingly on lld if it were the case).

As I understand it, IRELATIVE relocations are not resolved lazily by the dynamic loader so there would not need to be a variant PCS flag in this particular case. So I think it is likely binutils is right here. No harm in checking though.

Ah yes, IRELATIVE and (J[U]MP_SLOT referencing STT_GNU_IFUNC) relocations are eagerly resolved. Having a test will be good, too (which probably suggests to split the tests. You can leverage the newer utility split-file %s %t)

[zatrazz]

Updated patch based on previous comment:

• Use the split-file tool to enable multiple tests.

[zatrazz]

Both https://reviews.llvm.org/D93044 and https://reviews.llvm.org/D93235 have been approved, so the prerequisites of this patch are ok to land.

[MaskRay]

lld/test/ELF/aarch64-variant_pcs.s

6

Delete --soname=%t.so (you encode the absolute path %t into DT_SONAME)

If a shared object is not used for linking in the same test, it does not need DT_SONAME.

60

#---

Just use the same comment marker as other comments

[zatrazz]

lld/test/ELF/aarch64-variant_pcs.s

6

Ack.

60

Ack.

Should I resend a newer version?

[MaskRay]

lld/test/ELF/aarch64-variant_pcs.s

60

Yes.

Unlikely email based reviews (where I might feel bad once my patch reached v5, v6, ...), there is nothing bad uploading several versions on Phabricator. Reviewers and CCers will get an email but that happens for each comment as well, so they should have high tolerance...

[zatrazz]

Updated patch based on previous version.

[MaskRay]

Thanks! Would be good checking with @psmith and/or @nsz

lld/ELF/SyntheticSections.cpp

if (isMain && (in.relaPlt->isNeeded() || in.relaIplt->isNeeded())) {
     entries.push_back({DT_PLTRELSZ, addPltRelSz});
     switch (config->emachine) {
     case EM_MIPS:
       addInSec(DT_MIPS_PLTGOT, in.gotPlt);
       break;
     case EM_SPARCV9:
       addInSec(DT_PLTGOT, in.plt);
       break;
+    case EM_AARCH64:
+      if (llvm::find_if(in.relaPlt->relocs, [](const DynamicReloc &r) {
+           return r.type == target->pltRel &&
+                  r.sym->stOther & STO_AARCH64_VARIANT_PCS;
+          }) != in.relaPlt->relocs.end())
+        addInt(DT_AARCH64_VARIANT_PCS, 0);
+      LLVM_FALLTHROUGH;
     default:
       addInSec(DT_PLTGOT, in.gotPlt);
       break;
     }
     addInt(DT_PLTREL, config->isRela ? DT_RELA : DT_REL);
   }
 
   if (config->emachine == EM_AARCH64) {
if (sym->isLocal()) {
       eSym->setBindingAndType(sym->computeBinding(), sym->type);
       eSym->setVisibility(sym->visibility);
     }
 
     // The 3 most significant bits of st_other are used by OpenPOWER ABI.
     // See getPPC64GlobalEntryToLocalEntryOffset() for more details.
     if (config->emachine == EM_PPC64)
       eSym->st_other |= sym->stOther & 0xe0;
+    // The most significant bit of st_other is used by AArch64 ABI for the
+    // variant PCS.
+    else if (config->emachine == EM_AARCH64)
+      eSym->st_other |= sym->stOther & STO_AARCH64_VARIANT_PCS;
 
     eSym->st_name = ent.strTabOffset;
     if (isDefinedHere)
       eSym->st_shndx = getSymSectionIndex(ent.sym);
     else
       eSym->st_shndx = 0;
 
     // Copy symbol size if it is a defined symbol. st_size is not significant

lld/test/ELF/aarch64-variant_pcs.s

+# REQUIRES: aarch64
+# RUN: split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/test1 -o %t.o
+# RUN: ld.lld %t.o --shared -o %t.so
+# RUN: llvm-readelf --dynamic-table %t.so | FileCheck --check-prefix T1-PCSDYN %s
+# RUN: llvm-readelf --symbols %t.so | FileCheck --check-prefix T1-PCSSYM %s
+
+# T1-PCSDYN-NOT:  0x0000000070000005 (AARCH64_VARIANT_PCS) 0
+# T1-PCSSYM:      Symbol table '.dynsym'
+# T1-PCSSYM:      0 NOTYPE GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_func_global_def
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/test2 -o %t.o
+# RUN: ld.lld %t.o --shared -o %t.so
+# RUN: llvm-readelf --dynamic-table %t.so | FileCheck --check-prefix T2-PCSDYN %s
+# RUN: llvm-readelf --symbols %t.so | FileCheck --check-prefix T2-PCSSYM %s
+
+# T2-PCSDYN:      0x0000000070000005 (AARCH64_VARIANT_PCS) 0
+# T2-PCSSYM:      Symbol table '.dynsym'
+# T2-PCSSYM:      0 NOTYPE GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_func_global_def
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/test3 -o %t.o
+# RUN: ld.lld %t.o --shared -o %t.so
+# RUN: llvm-readelf --dynamic-table %t.so | FileCheck --check-prefix T3-PCSDYN %s
+# RUN: llvm-readelf --symbols %t.so | FileCheck --check-prefix T3-PCSSYM %s
+
+# T3-PCSDYN:      0x0000000070000005 (AARCH64_VARIANT_PCS) 0
+# T3-PCSSYM:      Symbol table '.dynsym'
+# T3-PCSSYM:      0 IFUNC  GLOBAL DEFAULT [VARIANT_PCS] UND   pcs_ifunc_global_def
+# T3-PCSSYM:      0 NOTYPE GLOBAL DEFAULT               [[#]] pcs_func_global_def
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/test4 -o %t.o
+# RUN: ld.lld %t.o --shared -o %t.so
+# RUN: llvm-readelf --dynamic-table %t.so | FileCheck --check-prefix T4-PCSDYN %s
+# RUN: llvm-readelf --symbols %t.so | FileCheck --check-prefix T4-PCSSYM %s
+
+# T4-PCSDYN-NOT:  0x0000000070000005 (AARCH64_VARIANT_PCS) 0
+# T4-PCSSYM:      Symbol table '.dynsym'
+# T4-PCSSYM:      0 IFUNC GLOBAL DEFAULT [VARIANT_PCS]  [[#]] pcs_ifunc_global_def
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/test5 -o %t.o
+# RUN: ld.lld %t.o --shared -o %t.so
+# RUN: llvm-readelf --symbols %t.so | FileCheck --check-prefix T5-PCSSYM %s
+
+# T5-PCSSYM:      Symbol table '.dynsym'
+# T5-PCSSYM:      0 NOTYPE  GLOBAL DEFAULT [VARIANT_PCS] UND   pcs_func_global_undef
+# T5-PCSSYM-NEXT: 0 NOTYPE  GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_func_global_def
+# T5-PCSSYM-NEXT: 0 IFUNC   GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_ifunc_global_def
+# T5-PCSSYM:      Symbol table '.symtab' contains 10 entries:
+# T5-PCSSYM:      0 NOTYPE  LOCAL  DEFAULT [VARIANT_PCS] [[#]] pcs_func_local
+# T5-PCSSYM-NEXT: 0 IFUNC   LOCAL  DEFAULT [VARIANT_PCS] [[#]] pcs_ifunc_local
+# T5-PCSSYM-NEXT: 0 NOTYPE  LOCAL  HIDDEN  [VARIANT_PCS] [[#]] pcs_func_global_hidden
+# T5-PCSSYM-NEXT: 0 IFUNC   LOCAL  HIDDEN  [VARIANT_PCS] [[#]] pcs_ifunc_global_hidden
+# T5-PCSSYM:      0 NOTYPE  GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_func_global_def
+# T5-PCSSYM-NEXT: 0 NOTYPE  GLOBAL DEFAULT [VARIANT_PCS] UND   pcs_func_global_undef
+# T5-PCSSYM-NEXT: 0 IFUNC   GLOBAL DEFAULT [VARIANT_PCS] [[#]] pcs_ifunc_global_def
+
+
+#--- test1
+## An object with a variant_pcs symbol but without a R_AARCH64_JMP_SLOT
+## should not generate a DT_AARCH64_VARIANT_PCS.
+.text
+.global pcs_func_global_def
+.variant_pcs pcs_func_global_def
+
+pcs_func_global_def:
+  ret
+
+#--- test2
+## An object with a variant_pcs symbol and with a R_AARCH64_JMP_SLOT
+## should generate a DT_AARCH64_VARIANT_PCS.
+.text
+.global pcs_func_global_def
+.variant_pcs pcs_func_global_def
+
+pcs_func_global_def:
+  bl pcs_func_global_def
+
+#--- test3
+## Same as before, but targeting a GNU IFUNC.
+.text
+.global pcs_ifunc_global_def
+.global pcs_func_global_def
+.variant_pcs pcs_ifunc_global_def
+.type pcs_ifunc_global_def, %gnu_indirect_function
+
+pcs_func_global_def:
+  bl pcs_ifunc_global_def
+
+#--- test4
+## An object with a variant_pcs symbol and with a R_AARCH64_IRELATIVE
+## should not generate a DT_AARCH64_VARIANT_PCS.
+.text
+.global pcs_ifunc_global_def
+.global pcs_func_global_def
+.variant_pcs pcs_ifunc_global_def
+.type pcs_ifunc_global_def, %gnu_indirect_function
+
+pcs_ifunc_global_def:
+  bl pcs_func_global_def
+
+#--- test5
+## Check if STO_AARCH64_VARIANT_PCS is kept on symbol st_other for both undef,
+## local, and hidden visibility.
+.text
+.global pcs_func_global_def, pcs_func_global_undef, pcs_func_global_hidden
+.global pcs_ifunc_global_def, pcs_ifunc_global_hidden
+.local pcs_func_local
+
+.hidden pcs_func_global_hidden, pcs_ifunc_global_hidden
+
+.type pcs_ifunc_global_def, %gnu_indirect_function
+.type pcs_ifunc_global_hidden, %gnu_indirect_function
+.type pcs_ifunc_local, %gnu_indirect_function
+
+.variant_pcs pcs_func_global_def
+.variant_pcs pcs_func_global_undef
+.variant_pcs pcs_func_global_hidden
+.variant_pcs pcs_func_local
+.variant_pcs pcs_ifunc_global_def
+.variant_pcs pcs_ifunc_global_hidden
+.variant_pcs pcs_ifunc_local
+
+pcs_func_global_def:
+pcs_func_global_hidden:
+pcs_func_local:
+pcs_ifunc_global_def:
+pcs_ifunc_global_hidden:
+pcs_ifunc_local:
+  ret