[PATCH] D48247: lld: add experimental support for SHT_RELR sections.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking good.



================
Comment at: ELF/Driver.cpp:887-888
   }
+  Config->UseAndroidRelrTags = Args.hasFlag(
+      OPT_use_android_relr_tags, OPT_no_use_android_relr_tags, false);
 
----------------
Move this before the code to initialize Config->UnresolvedSymbols.


================
Comment at: ELF/Relocations.cpp:715
 
+static void addRelativeReloc(InputSectionBase *IS, uint64_t OffsetInSec,
+                             Symbol *Sym, int64_t Addend, RelExpr Expr,
----------------
This function needs comment. (Any non-trivial code needs comment so that the code can be understood by first-time readers.)


================
Comment at: ELF/SyntheticSections.cpp:1711
+template <class ELFT> RelrSection<ELFT>::RelrSection() {
+  this->Entsize = sizeof(Elf_Relr);
+}
----------------
Elf_Relr -> Config->Wordsize


================
Comment at: ELF/SyntheticSections.cpp:1761
+  typename std::vector<uint64_t>::iterator Curr = Offsets.begin();
+  while (Curr != Offsets.end()) {
+    uint64_t Current = *Curr;
----------------
This loop looks odd to me because it doesn't reflect the logic that this loop should implement. Currently, the loop looks like this

  while () {
     do_something();
     if (...)
      add_a_leading_address_entry();
     else
      add_a_bitmap();
  }

However, IIUC, the straightforward translation of the logic in the pseudo code should look like this:

  while (there's remaining relocation) {
    add_a_first_address(the first remaining relocation);
    while (more relocations can be folded)
      add_a_bitmap(foldable relocations);
  }

As a result, I think you had to write non-obvious code to determine whether you need to add a leading address entry or a bitmap. I believe this code can be (and perhaps should be) improved. That said, I'm fine if you submit this code as-is. I'll try to address that as a follow-up patch.


================
Comment at: ELF/SyntheticSections.cpp:1783
+
+    Elf_Relr R;
+    if (Bits == 0) {
----------------
Please use `typename ELFT::uint` instead of Elf_Relr.


================
Comment at: ELF/SyntheticSections.h:534
+
+template <class ELFT> class RelrSection final : public RelrBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
Please write a comment as to what this class represents.


================
Comment at: ELF/SyntheticSections.h:547
+private:
+  std::vector<Elf_Relr> RelrRelocs;
+};
----------------
Use `typename ELFT::uint` instead of ELF_Relr.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247