This is an archive of the discontinued LLVM Phabricator instance.

[ELF] AArch64: Handle DT_AARCH64_VARIANT_PCS
ClosedPublic

Authored by zatrazz on Dec 10 2020, 8:39 AM.

Details

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.

Diff Detail

Event Timeline

zatrazz created this revision.Dec 10 2020, 8:39 AM
zatrazz requested review of this revision.Dec 10 2020, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 8:39 AM

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 added a subscriber: nsz.Dec 10 2020, 11:32 AM

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 added inline comments.Dec 10 2020, 11:49 AM
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.

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 added inline comments.Dec 10 2020, 12:03 PM
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 added inline comments.Dec 10 2020, 12:05 PM
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 this revision to Diff 311180.Dec 11 2020, 5:12 AM

Updated patch based on previous comments.

MaskRay added inline comments.Dec 11 2020, 9:12 AM
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 added inline comments.Dec 11 2020, 10:43 AM
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 added inline comments.Dec 11 2020, 10:53 AM
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 this revision to Diff 311295.Dec 11 2020, 11:57 AM

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).

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.

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 this revision to Diff 311887.Dec 15 2020, 6:28 AM

Updated patch based on previous comment:

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

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 added inline comments.Dec 16 2020, 9:53 AM
lld/test/ELF/aarch64-variant_pcs.s
5

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.

59

#---

Just use the same comment marker as other comments

zatrazz added inline comments.Dec 16 2020, 9:59 AM
lld/test/ELF/aarch64-variant_pcs.s
5

Ack.

59

Ack.

Should I resend a newer version?

MaskRay added inline comments.Dec 16 2020, 10:13 AM
lld/test/ELF/aarch64-variant_pcs.s
59

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 this revision to Diff 312254.Dec 16 2020, 10:21 AM

Updated patch based on previous version.

MaskRay accepted this revision.Dec 16 2020, 10:32 AM

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

This revision is now accepted and ready to land.Dec 16 2020, 10:32 AM
This revision was automatically updated to reflect the committed changes.