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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: ELF/Config.h:117-118
   bool AndroidPackDynRelocs = false;
+  bool RelrPackDynRelocs = false;
+  bool AndroidRelrPackDynRelocs = false;
   bool ARMHasBlx = false;
----------------
Can you add an enum class for the `--pack-dyn-relocs` flag instead of adding these bools?


================
Comment at: ELF/SyntheticSections.cpp:1741
+  // Details of the encoding are described in this post:
+  //   https://groups.google.com/d/msg/generic-abi/bX460iggiKg/Pi9aSwwABgAJ
+  Word Base = 0;
----------------
Please also include a brief description of the encoding as a comment in the code.


================
Comment at: ELF/SyntheticSections.cpp:1747
+    if (Current%2 != 0) {
+      error("odd offset for RELR relocation (" + Twine(Current) + "), " +
+            "pass --pack-dyn-relocs=none to disable RELR relocations");
----------------
I think you can avoid needing to error on this by only creating RELR relocations for relocations at even offsets in sections with sh_addralign >= 2. In practice I think this would be almost every section that might contain a relative relocation.


================
Comment at: ELF/SyntheticSections.cpp:1755
+    typename std::vector<Word>::iterator Next = Curr;
+    if ((Base > 0) && (Base <= Current)) {
+      while (Next != Offsets.end()) {
----------------
Remove unnecessary parens.


================
Comment at: ELF/SyntheticSections.cpp:1759
+        // If Next is too far out, it cannot be folded into Curr.
+        if (Delta >= (NBits * WordSize))
+          break;
----------------
Remove unnecessary parens.


================
Comment at: ELF/SyntheticSections.cpp:1763
+        // be folded into Curr.
+        if ((Delta % WordSize) != 0)
+          break;
----------------
Remove unnecessary parens.


================
Comment at: ELF/SyntheticSections.h:477
                 RelType Type);
-  void addReloc(const DynamicReloc &Reloc);
+  virtual void addReloc(const DynamicReloc &Reloc);
   bool empty() const override { return Relocs.empty(); }
----------------
Instead of making this virtual and teaching RelocationBaseSection about RELR I think we should add logic here:
http://llvm-cs.pcc.me.uk/tools/lld/ELF/Relocations.cpp#797
to directly add the relocation to RELR if applicable.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247