[PATCH] D100490: [ELF] Check the Elf_Rel addends for dynamic relocations

Fri Apr 23 04:01:36 PDT 2021

arichardson added a comment.

In D100490#2711740 <https://reviews.llvm.org/D100490#2711740>, @peter.smith wrote:

> I've had a quick read through and I like where this is going. There's a lot to check through and I've run out of time today. I've left a couple of small suggestions. Will need to come back and look in more detail, probably next week to see if I've got anything else to add.

Thanks for reviewing! I noticed there is one larger issue that needs to be addressed: If we use -z rel / --apply-dynamic-relocs, this may report false-positive errors for architectures with missing `getImplicitAddend()` cases (e.g. AArch64).
I wonder if there needs to be a per-architecture default for checking the addends so that we can gradually enable this by default (starting with ARM, MIPS, and i386). I can then split out the X86_64 `getImplicitAddend()` changes to a separate review.
Maybe there should be an internal testing option `--[no-]check-dynamic-reloc-addends` until `getImplicitAddend()` is implemented for all architectures?

================
Comment at: lld/ELF/SyntheticSections.h:525
+  /// Add a dynamic relocation against \p sym with an optional addend.
+  void addReloc(RelType dynType, InputSectionBase *isec, uint64_t offsetInSec,
+                Symbol &sym, int64_t addend = 0,
----------------
peter.smith wrote:
> Is there scope to rename this so that it is easier to see at the call site? Like addRelativeReloc(). For example addSymbolReloc() or addAgainstSymbolReloc();
Sounds good, will do in next version of this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100490