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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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



================
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);
----------------
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.` 


================
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
----------------
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});
}
```


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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156882/new/

https://reviews.llvm.org/D156882