[PATCH] D156882: [AArch64][ELF][lld] Support dynamic R_AARCH64_AUTH_* relocations
Daniil Kovalev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 14 08:31:34 PDT 2023
kovdan01 added inline comments.
================
Comment at: lld/ELF/Relocations.cpp:1442
+ if (config->emachine == EM_AARCH64 && type == R_AARCH64_AUTH_ABS64) {
+ assert(RelTy::IsRela); // Assume that AArch64 uses rela
+ std::lock_guard<std::mutex> lock(relocMutex);
----------------
peter.smith wrote:
> 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.`
It makes sense, changed the comment.
================
Comment at: lld/ELF/Relocations.cpp:1450
+ part.relaDyn->addSymbolReloc(type, *sec, offset, sym, addend, type);
+ } else {
+ // Check if an implicit addend to be stored is a 32-bit integer since only
----------------
peter.smith wrote:
> 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});
> }
> ```
This is definitely better, changed, thanks.
================
Comment at: lld/ELF/Relocations.cpp:1455
+ // For comments regarding relr, see addRelativeReloc.
+ if (part.relrAuthDyn && sec->addralign >= 2 && offset % 2 == 0 &&
+ isInt<32>(sym.getVA(addend))) {
----------------
peter.smith wrote:
> 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.
Got it, thanks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156882/new/
https://reviews.llvm.org/D156882
More information about the llvm-commits
mailing list