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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Driver.cpp:859
+      Config->RelrPackDynRelocs = true;
+    else if (S == "android-relr") {
+      Config->RelrPackDynRelocs = true;
----------------
nit: if you add {} to one of `else if`, please add it to all `if` and `else if`s.


================
Comment at: ELF/SyntheticSections.cpp:1446
                                      RelType Type) {
-  // Write the addends to the relocated address if required. We skip
-  // it if the written value would be zero.
-  if (Config->WriteAddends && (Expr != R_ADDEND || Addend != 0))
+  // Write the addends to the relocated address if required.
+  bool WriteAddend = Config->WriteAddends;
----------------
I don't get the meaning of this comment. What does `if required` actually mean? I think that should be answered in the comment. Actually, I'd think instead of writing comment for each expression, you probably should write a function comment to give readers a big picture as to what this function is supposed to do.


================
Comment at: ELF/SyntheticSections.cpp:1461
 void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
-  if (Reloc.Type == Target->RelativeRel)
-    ++NumRelativeRelocs;
-  Relocs.push_back(Reloc);
+  if (InX::RelrDyn && Reloc.Type == Target->RelativeRel)
+    InX::RelrDyn->addReloc(Reloc);
----------------
Add {}


================
Comment at: ELF/SyntheticSections.cpp:1717
+
+template <class ELFT>
+bool RelrPackedRelocationSection<ELFT>::updateAllocSize() {
----------------
Please write a function comment so that the first-time reader can understand your code without hassle.


================
Comment at: ELF/SyntheticSections.cpp:1718
+template <class ELFT>
+bool RelrPackedRelocationSection<ELFT>::updateAllocSize() {
+  size_t OldSize = RelrRelocs.size();
----------------
This function seems a bit too long. Please consider splitting this into multiple small functions.


================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
In lld, we try to keep the class hierarchy as shallow as possible. Please avoid class inheritance when possible.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247