[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;
----------------
rahulchaudhry wrote:
> 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.
> 
> 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.
I was thinking that you would add a line to the end of this function http://llvm-cs.pcc.me.uk/tools/lld/ELF/Driver.cpp#928 like this:
```
Config->Relr = Config->PackDynRelocs == Relr || Config->PackDynRelocs == AndroidRelr;
```
then have the code check `Config->Relr` in most places.

> 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.

That doesn't look like the current behaviour though?

That said, can we just make `--pack-dyn-relocs=android-relr` mean "use SHT_ANDROID_RELR and SHT_ANDROID_REL/SHT_ANDROID_RELA"? Is there any case in which it would be useful to have SHT_ANDROID_RELR combined with unpacked SHT_REL/SHT_RELA?


================
Comment at: ELF/Relocations.cpp:786
     if (!IsPreemptibleValue) {
-      InX::RelaDyn->addReloc(Target->RelativeRel, &Sec, Offset, &Sym, Addend,
-                             Expr, Type);
+      if (InX::RelrDyn)
+        InX::RelrDyn->addReloc(Target->RelativeRel, &Sec, Offset, &Sym, Addend,
----------------
Re SyntheticSections.cpp:1752 comment I think this can be something like `if (InX::RelrDyn && Sec.Alignment >= 2 && Offset % 2 == 0)` now.


================
Comment at: ELF/SyntheticSections.cpp:1453-1464
+  // Add a dynamic relocation that might need an addend. This takes care of
+  // writing the addend to the output section if needed. We need to write
+  // the addend if either:
+  //   - Config->WriteAddends is true, or
+  //   - We're writing a RELR relocation.
+  // We can skip writing the addend if the written value would be zero.
+  bool WriteAddend = Config->WriteAddends;
----------------
I don't think you need to add logic here at all. Instead, you can add the static relocation directly to the input section in Relocations.cpp when you decide to add the relocation to RELR.


================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
rahulchaudhry wrote:
> 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.
> 
It doesn't really fit though because `RelrPackedRelocationSection` is unlike the other sections in that it can only store one specific type of relocation. With the change to add relocations directly to `InX::RelrDyn` I think you can just make this class derive from `SyntheticSection` and have it store a vector of (section, offset) pairs instead of `DynamicReloc`s.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247