[PATCH] D156882: [AArch64][ELF][lld] Support dynamic R_AARCH64_AUTH_* relocations

[Daniil Kovalev via Phabricator via llvm-commits]

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