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

[Rahul Chaudhry via Phabricator via llvm-commits]

rahulchaudhry added inline comments.


================
Comment at: ELF/Config.h:117-118
   bool AndroidPackDynRelocs = false;
+  bool RelrPackDynRelocs = false;
+  bool AndroidRelrPackDynRelocs = false;
   bool ARMHasBlx = false;
----------------
pcc wrote:
> Can you add an enum class for the `--pack-dyn-relocs` flag instead of adding these bools?
I started on this, but it got clumsy very fast.
The problem is that these bools are not mutually exclusive.
e.g. for --pack-dyn-relocs=android-relr, both RelrPackDynRelocs and AndroidRelrPackDynRelocs are set to true. Most places look at RelrPackDynRelocs, and AndroidRelrPackDynRelocs is only checked to decide which tags to emit. This gets messy when you can only set an enum variable to Relr or AndroidRelr. Many places end up needing to check for both values.
Another example is that --pack-dyn-relocs=android-relr and --pack-dyn-relocs=android are not mutually exclusive. It's fine to have both on the command line, and the effect should be to move relative relocations to SHT_RELR, and pack the remaining relocations in SHT_REL/SHT_RELA using AndroidPackedRelocationSection.



================
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);
----------------
ruiu wrote:
> Add {}
This logic is now moved to Relocations.cpp


================
Comment at: ELF/SyntheticSections.cpp:1718
+template <class ELFT>
+bool RelrPackedRelocationSection<ELFT>::updateAllocSize() {
+  size_t OldSize = RelrRelocs.size();
----------------
ruiu wrote:
> This function seems a bit too long. Please consider splitting this into multiple small functions.
Ack.
The main logic in the function is computing the bitmap, which is 15 lines including comments beginning with 'if ((Base > 0) && (Base <= Current))'
The rest of the function is setting up common types and consts, sorting the offsets, and iterating over them.
I don't see an easy way to split it up without duplicating the common type and const declarations or moving them into the class.



================
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");
----------------
pcc wrote:
> 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.
I guess the check would now be in Relocations.cpp before calling InX::RelrDyn->addReloc().
Any hint on what the check would be? I don't see any other obvious place in that file checking for section alignment.
I think this check should still be preserved, just in case something odd ends up here (maybe convert it to fatal?)



================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
ruiu wrote:
> In lld, we try to keep the class hierarchy as shallow as possible. Please avoid class inheritance when possible.
Ack. In this case, the existing hierarchy is
- RelocationBaseSection inherits from SyntheticSection
- RelocationSection and AndroidPackedRelocationSection both inherit from RelocationBaseSection

RelrPackedRelocationSection fits right here in the hierarchy with RelocationSection and AndroidPackedRelocationSection as siblings.



Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247