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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

The Arm relocations look correct to me. I've got a few suggestions, but no objections.



================
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
----------------
Would it be possible to assert that we are not linking with -r and make sure we don't call checkDynRelAddends() for ld -r?


================
Comment at: lld/ELF/OutputSections.cpp:566
+        errorOrWarn(
+            getErrorLocation(relocTarget) +
+            "wrote incorrect addend for dynamic relocation: value is 0x" +
----------------
Although harder to read than an instance of the error message, it took a second reading to work out which value was the addend. Perhaps "wrote incorrect addend value 0x... but expected 0x..."


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100490