[PATCH] D42843: Ensure that Elf_Rel addends are always written for dynamic relocations

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1208
+                                     uint64_t OffsetInSec, Symbol *Sym) {
+  addReloc({DynType, InputSec, OffsetInSec, false, Sym, 0}, DynType, R_INVALID);
+}
----------------
Do I understand it correctly that, when you call the second `addReloc` (by the way it is better to give the two functions different name), `if (!Config->IsRela)` part won't be executed? If so, can this function just be

  if (Reloc.Type == Target->RelativeRel)
    ++NumRelativeRelocs;
  Relocs.push_back(Reloc);

?


================
Comment at: ELF/SyntheticSections.cpp:1222-1224
+    // TODO: should we also write zero addends? Since this is the common case
+    // it will probably slow down adding dynamic relocations significantly and
+    // I can't think of a case where we would turn a non-zero addend into zero.
----------------
I agree with you that this is a nice optimization. Please remove this TODO and just say that we skip addend 0 to reduce the number of dynamic relocations.


================
Comment at: ELF/SyntheticSections.h:319-325
+  // Return the value that should be written to the Elf_Rela r_addend field.
+  // This is not the same as the addend used when creating this dynamic
+  // relocation since we may have converted a static relocation against a
+  // non-preemptible symbol into a relocation against the load address. In that
+  // case we write the virtual address of the symbol plus the addend into the
+  // r_addend field.
+  int64_t getRelaAddend() const;
----------------
nit: add blank lines before and after this block so that it is obvious that this comment describes getRelaAddend.


================
Comment at: ELF/SyntheticSections.h:335-336
+  // address and write the symbol virtual address as the addend
+  // TODO: I found `UseSymVA` to be non-obvious as there are no comments, how
+  // about `IsAgainstLoadAddr`?
   bool UseSymVA;
----------------
This TODO should be a new code review request rather than a TODO in source code.


================
Comment at: ELF/SyntheticSections.h:379
+  // against the load address. In that case the addend will be the virtual
+  // address of the symbol and must be written to the input section in the
+  // case of REL output
----------------
input -> output?


================
Comment at: test/ELF/convert-rel-rela-addends.s:110
+.quad 0xabcdef0012345678
\ No newline at end of file

----------------
Please make sure the last byte of a text file is '\n'.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42843