[PATCH] D100490: [ELF] Refactor DynamicReloc to fix incorrect relocation addends

[Alexander Richardson via Phabricator via llvm-commits]

arichardson marked 5 inline comments as done.
arichardson added a comment.

Apologies for the delay, I was away on holiday and then working on non-LLVM tasks for the past two months. I've uploaded a new version that should hopefully address the remaining comments.



================
Comment at: lld/ELF/InputSection.cpp:1021
 
+    if (rel.expr == R_ADDEND) {
+      target->relocate(bufLoc, rel, rel.addend);
----------------
MaskRay wrote:
> arichardson wrote:
> > This is needed now to avoid UB caused by`*rel.sym` if rel.sym is NULL.
> If I delete this block, all tests pass.
I got UBSan failures before but it might just be because of further local changes after this patch. Will drop if it isn't needed after all.
I believe the sym == nullptr case can only happen for the MIPS GOT and that shouldn't call relocateAlloc right now as it writes addends explicitly (I had a follow-up change to use relocateAlloc instead, so it's probably only needed for that patch).


================
Comment at: lld/ELF/SyntheticSections.cpp:1604
+  }
+  llvm_unreachable("");
 }
----------------
MaskRay wrote:
> This is unneeded
I feel like I needed this to make GCC happy. I can remove it and see if CI complains.


================
Comment at: lld/ELF/SyntheticSections.h:464
+  /// multi-GOT implementation.
   DynamicReloc(RelType type, const InputSectionBase *inputSec,
                uint64_t offsetInSec, const OutputSection *outputSec,
----------------
MaskRay wrote:
> This constructor is only called once, in MIPS multi-GOT code. The constructor should be deleted.
The other constructors can't be used here since they are missing the `OutputSection*` parameter.
I could add it to the constructor above if you prefer, but since it's only needed for MIPS I think having a separate one is slightly cleaner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100490