This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][ELF][lld] Support AUTH relocations and AUTH ELF marking
Needs ReviewPublic

Authored by kovdan01 on Aug 2 2023, 5:29 AM.

Details

Summary

This patch adds lld support for:

Co-authored-by: Peter Collingbourne <peter@pcc.me.uk>

Diff Detail

Event Timeline

kovdan01 created this revision.Aug 2 2023, 5:29 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kovdan01 requested review of this revision.Aug 2 2023, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 5:29 AM

Just a few small comments from me. Overall approach looks good to me. I expect MaskRay will want to take a look too.

lld/ELF/Relocations.cpp
1442

With -z rel as used by Fuchsia it can write out dynamic REL relocations but I think think this particular case is always from relocatable object files, which from clang/llvm-mc at least are always RELA.

Suggest changing the comment to:
// Assume relocations from relocatable objects are RELA.

1450

I think LLD would normally make this a flat if, else if, else. I've suggested some simplifications and comment changes below.

else if (part.relrAuthDyn && isInt<32>(sym.getVA(addend)))) {
  // Implicit addend is below 32-bits so we can use the compressed
  // relative relocation section. The R_AARCH64_AUTH_RELATIVE
  // has a smaller addend fielf as bits [63:32] encode the signing-schema.
  sec->addReloc({expr, type, offset, addend, &sym});
  part.relrAuthDyn->relocsVec[parallel::getThreadIndex()].push_back(
            {sec, offset});
} else {
   part.relaDyn->addReloc({R_AARCH64_AUTH_RELATIVE, sec, offset,
                            DynamicReloc::AddendOnlyWithTargetVA, sym,
                            addend, R_ABS});
}
1455

All AArch64 instructions require 4-byte alignment so sec->addralign >=2 and offset %2 == 0 will always be true otherwise code will get a fault at runtime.

For llvm it is unfortunate that a .section directive won't implicitly add the alignment so the user has to remember to add an align directive manually. This may lead to some assembler files having sections with sec->addralign being 1, but as long as these don't follow data sections (all AArch64 instructions are 4-bytes in size) then they effectively be 4-byte aligned.

kovdan01 updated this revision to Diff 549959.Aug 14 2023, 8:31 AM
kovdan01 marked 3 inline comments as done.
  • Rebase to current mainline and apply changes from D156505
  • Fix review comments
lld/ELF/Relocations.cpp
1442

It makes sense, changed the comment.

1450

This is definitely better, changed, thanks.

1455

Got it, thanks

@MaskRay would be glad to see your comment on proposed changes

MaskRay added inline comments.Aug 16 2023, 9:34 PM
lld/ELF/Driver.cpp
1681

nopack-relative-auth-relocs is untested.

lld/ELF/Writer.cpp
402
lld/test/ELF/aarch64-ptrauth.s
4

omit -unknown-linux as this applies to generic ELF.

158

delete trailing blank line

llvm/include/llvm/BinaryFormat/DynamicTags.def
137 ↗(On Diff #546424)

This suggests that the spec DT_AARCH64_AUTH_RELR* should be fixed first:) https://github.com/ARM-software/abi-aa/pull/214 has changed the values now.

kovdan01 updated this revision to Diff 551158.EditedAug 17 2023, 8:53 AM
kovdan01 marked 4 inline comments as done.

Fix comments by @MaskRay

TODO: update the revision when https://reviews.llvm.org/D156505 is updated

lld/ELF/Driver.cpp
1681

It seems that it is, see lld/test/ELF/aarch64-ptrauth. Am I mistaken?

lld/ELF/Writer.cpp
402

Fixed

lld/test/ELF/aarch64-ptrauth.s
4

Fixed

158

Fixed

llvm/include/llvm/BinaryFormat/DynamicTags.def
137 ↗(On Diff #546424)

Fixed, thanks!

kovdan01 updated this revision to Diff 551523.Aug 18 2023, 8:25 AM

Fix tests

kovdan01 updated this revision to Diff 554796.Aug 30 2023, 12:07 PM

Rebase to current mainline

@MaskRay A kind reminder regarding the revision. Would be glad to see your comments.

@MaskRay Would be glad to see your comments on the revision. It seems that the issues mentioned previously are resolved now.

MaskRay added inline comments.Sep 17 2023, 6:17 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
2072 ↗(On Diff #554796)

These llvm-readobj changes should be moved to D158574

2087 ↗(On Diff #554796)

immediately invoked function expression

2103 ↗(On Diff #554796)

if else if ... is better than immediately invoked function expression here

6233 ↗(On Diff #554796)

processor-specific constants need to check isAArch64.

kovdan01 updated this revision to Diff 557428.Sep 27 2023, 5:27 PM
kovdan01 marked 4 inline comments as done.
kovdan01 retitled this revision from [AArch64][ELF][lld] Support dynamic R_AARCH64_AUTH_* relocations to [AArch64][ELF][lld] Support AUTH relocations and AUTH ELF marking.
kovdan01 edited the summary of this revision. (Show Details)

@MaskRay Resolved the issues mentioned and moved the patch to https://github.com/llvm/llvm-project/pull/72714. Please consider reviewing it there. Thanks!

llvm/tools/llvm-readobj/ELFDumper.cpp
2072 ↗(On Diff #554796)

Done

2087 ↗(On Diff #554796)

Switched to if else if

2103 ↗(On Diff #554796)

Fixed

6233 ↗(On Diff #554796)

Added a warning, thanks.

(Apologies that I'll not check emails very frequently in the next two weeks. )