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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a comment.

In D100490#2690103 <https://reviews.llvm.org/D100490#2690103>, @MaskRay wrote:

> The getImplicitAddend enhancement (`-z rel`) can be moved to a separate patch, but the dynamic only relocation types (JUMP_SLOT, RELATIVE, IRELATIVE, etc) should be removed since they can't be input.
>
> useSymVA is subtle and I need time to grok it. I wonder whether the new member can improve https://bugs.llvm.org/show_bug.cgi?id=47009 TLSDESC for -z rel?

Regarding the JUMP_SLOT, RELATIVE, IRELATIVE relocations: I could add a separate `target->getImplicitDynRelAddend()` that handles the dynamic-only relocation, so that we still get an error message if we see one of the dynamic-only relocations in an input file?

It might also be nice to check the value of the JUMP_SLOT relocations (probably in a follow-up patch) instead of skipping them. This should be easy once the DynamicReloc useSymVA member been changed to an enum.

Longer-term I think it would be nice if the implicit addend code could be moved to LLVM instead so that llvm-readelf/llvm-readobj can also make use of it.



================
Comment at: lld/ELF/OutputSections.cpp:548
+    const auto *sec = dyn_cast<RelocationBaseSection>(sections[i]);
+    // When linking with -r we might also call this function for input
+    // .rel[a].data sections. In this case we don't check the addend since we
----------------
peter.smith wrote:
> Would it be possible to assert that we are not linking with -r and make sure we don't call checkDynRelAddends() for ld -r?
Yes, that sounds like a better approach, will update.


================
Comment at: lld/ELF/OutputSections.cpp:567
+            getErrorLocation(relocTarget) +
+            "wrote incorrect addend for dynamic relocation: value is 0x" +
+            utohexstr(writtenAddend) + " but expected 0x" + utohexstr(addend) +
----------------
peter.smith wrote:
> If I've understood this correctly if we get here then it is an internal LLD error rather than a user error? If I'm right it would be useful to prefix with something along the lines "Internal error, please contact your supplier etc." Otherwise if I'm just using LLD I might wonder if it is something that I've done wrong.
Yes, this should only ever be triggered if there is either a missing case in `target->getImplicitAddend()` (i.e. using `-z rel` on an architecture where getImplicitAddend() is not implemented) or if we failed to write the implicit addend correctly.


================
Comment at: lld/ELF/SyntheticSections.h:467
   bool useSymVA;
+  // If true, the relocation will be against sym rather than the load address
+  // (even if useSymVA is set). This should be set to true for relocations that
----------------
peter.smith wrote:
> I found it difficult from the comments what the interaction between the combinations of useSymVA and mustRetainSymbol are. Especially reading the two comments at the same time.
> 
> Is there scope for combining the two booleans into 1 enumeration, essentially capturing the valid combinations with a name? I think once set they aren't changed.
It also took me a long time to understand what is going on here. Will submit a new version that uses an enum instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100490