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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:1021
 
+    if (rel.expr == R_ADDEND) {
+      target->relocate(bufLoc, rel, rel.addend);
----------------
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.


================
Comment at: lld/ELF/SyntheticSections.cpp:1604
+  }
+  llvm_unreachable("");
 }
----------------
This is unneeded


================
Comment at: lld/ELF/SyntheticSections.h:432
+    /// of computeAddend().
+    RelativeNoSymbol,
+    /// This dynamic relocation will not be against the symbol but will instead
----------------
> is a relative relocation

Not accurate. 
tlsModuleIndexRel uses RelativeNoSymbol, however, the DTPMOD relocation type has nothing to do with R_*_RELATIVE.

> /// This dynamic relocation 

A bit verbose to repeat this for every value. Just omit the phrase?

Perhaps rename to `AddendOnly`, with a comment like `/// Use 0 as the symbol value ...`




================
Comment at: lld/ELF/SyntheticSections.h:437
+    /// final Elf_Rel/Elf_Rela addend.
+    RelativeWithTargetVA,
+    /// This dynamic relocation references symbol #sym from the dynamic symbol
----------------
> be a relative relocation

Not accurate. TLS-IE emitted dynamic `R_X86_64_TPOFF64` uses this kind as well.

Perhaps AddendOnlyWithTargetVA?

`/// Use 0 as the symbol value ... The addend is computed with getRelocTargetVA().`

No need to emphasize Elf_Rel/Elf_Rela/


================
Comment at: lld/ELF/SyntheticSections.h:464
+  /// multi-GOT implementation.
   DynamicReloc(RelType type, const InputSectionBase *inputSec,
                uint64_t offsetInSec, const OutputSection *outputSec,
----------------
This constructor is only called once, in MIPS multi-GOT code. The constructor should be deleted.


================
Comment at: lld/ELF/SyntheticSections.h:473
   uint32_t getSymIndex(SymbolTableBaseSection *symTab) const;
+  bool needsDynSymIndex() const { return sym && kind != RelativeWithTargetVA; }
 
----------------
Possible to use `kind` only and don't check `sym`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100490