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

Thu Jun 28 18:35:05 PDT 2018

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:
> > 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?
> I think it would be better to keep the two packing modes independent:
> - For debugging, someone might want to enable SHT_ANDROID_RELR but not SHT_ANDROID_RELA, or vice-versa.
> - At some later date, Android might decide to remove support for the custom (non-standard) SHT_ANDROID_RELA packing. This becomes cumbersome if SHT_ANDROID_RELR implies SHT_ANDROID_RELA.
> 
> I added another recognized form for this option: "android+relr", which is a combination of "android" and "android-relr".
> The naming is a bit confusing now. The "+" means plus/addition, but the "-" means hyphen (not minus/subtraction).
> Maybe "android-relr" should be renamed to "androidrelr" without the hyphen?
> 
> Also added an enum class, but that did not improve things much.
> The three bools are still there, but now they're set in Driver.cpp:setConfigs() using the enum value in Config->PackDynRelocs.
> 
Seems reasonable enough, and if we're going to go that way I think I'm convinced that you don't need the enum class.

I think you can avoid needing to have both `android-relr` and `android+relr` by having the flags look like this:
- `--pack-dyn-relocs=android` -- use Android packing
- `--pack-dyn-relocs=relr` -- use RELR packing
- `--pack-dyn-relocs=android+relr` -- do both of the above
- `--use-android-relr-tags` -- use SHT_ANDROID_RELR tag instead of SHT_RELR

================
Comment at: ELF/Relocations.cpp:736
+
+  RelocationBaseSection *RelSec = InX::RelaDyn;
+  if (InX::RelrDyn && Type == Target->RelativeRel && InX::Got->Alignment >= 2 &&
----------------
rahulchaudhry wrote:
> After logic to add relocations to RelrDyn was moved to Relocations.cpp, I noticed that chrome binary still had ~20,000 relative relocations left over in RelaDyn.
> This is the place where those remaining relocations were coming from.
> So moving this logic to the callers of addReloc() (instead of making addReloc virtual) does result in duplicating this logic.
> 
> There are a few other places that add RelativeRel to RelaDyn, but they're MipsGotSection::build(), so I assume they're mips specific.
> I have not put the logic there yet, so those relocations may still end up in RelaDyn instead of RelrDyn.
> 
You could always add a function `addRelativeReloc` and call it from both places as well as the places in the MIPS code.

================
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;
----------------
rahulchaudhry wrote:
> pcc wrote:
> > 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.
> There are at least two places where RelSec is set to either RelaDyn or RelrDyn. This logic for writing the addends would need to be in both places. There might be more places in the future, and it becomes all too easy (i.e. errorprone) to add a relocation to RelrDyn, but forget to write the addend for it in the section.
> I think keeping this logic in one place is desirable.
> 
> There are at least two places where RelSec is set to either RelaDyn or RelrDyn. 

Those places become a single place if you introduce `addRelativeReloc`. If for whatever reason new code ends up calling `InX::RelaDyn->addReloc` (although I don't see why since the author of that code would probably follow existing code that calls `addRelativeReloc`), that wouldn't be an error, it would just mean that that relocation wouldn't be compressed.

================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
rahulchaudhry wrote:
> pcc wrote:
> > 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.
> Deriving RelrPackedRelocationSection directly from SyntheticSection has several consequence:
> 
> - It cannot use the addReloc() functions from RelocationBaseSection. It'd need its own addReloc() and a vector of (section, offset) pairs like you suggested. This affects all call sites that do RelSec->addReloc(). Currently RelSec can be InX::RelaDyn or InX::RelrDyn and the same addReloc() call works fine for both. I think there's some value to storing DynamicRelocs the same way for all types of relocation sections.
> 
> - RelrPackedRelocationSection needs to be a template on <class ELFT> (similar to RelocationSection and AndroidPackedRelocationSection). RelocationBaseSection is not templated, and as such, RelaDyn, RelaPlt, etc are all declared as RelocationBaseSection* in struct InX. Currently RelrDyn is also in struct InX as RelocationBaseSection*. If it inherits directly from SyntheticSection, it would need to be moved to struct In<ELFT>. This would mean all references to InX::RelrDyn would have to converted to In<ELFT>::RelrDyn. All this makes using or referencing RelrDyn very unlike using or referencing RelaDyn when it is conceptually very similar.
> 
> - On top of the above, there are at least some places where InX::RelrDyn is referenced, but class ELFT is not available. Converting to In<ELFT>::RelrDyn would mean turning those functions into template versions with <class ELFT> as well. This can have a cascading effect, reversing some of the detemplating effort that I see has taken place in recent past.
> 
> I am not convinced that RelrPackedRelocationSection is so unlike the other classes for relocation sections that it doesn't fit in the heirarchy here and it's worth going through all this trouble to avoid inheriting from RelocationBaseSection.
> 
> This affects all call sites that do RelSec->addReloc()

Only the one in `addRelativeReloc`.

> RelrPackedRelocationSection needs to be a template on <class ELFT> (similar to RelocationSection and AndroidPackedRelocationSection). 

Yes, that's annoying. You can solve it in the same way as we avoid templating `RelocationBaseSection` on ELFT: introduce a `RelrBaseSection` which stores the (section, offset) pairs and derive `RelrSection<ELFT>` from it.

> I am not convinced that RelrPackedRelocationSection is so unlike the other classes for relocation sections that it doesn't fit in the heirarchy

>From my perspective it's not so much a matter of fitting in a hierarchy but using appropriate data structures. There's no need to use a data structure for arbitrary dynamic relocations when a simpler one would suffice.

================
Comment at: test/ELF/pack-dyn-relocs.s:1
 // REQUIRES: arm, aarch64

----------------
Please add a test showing that misaligned relative relocations end up in `.rel.dyn` or `.rela.dyn`.

Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247