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

Rahul Chaudhry via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 29 14:59:14 PDT 2018


rahulchaudhry marked 5 inline comments as done.
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:
> 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
Added new flag --use-android-relr-tags.



================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+  typedef typename ELFT::Relr Elf_Relr;
----------------
pcc wrote:
> 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.
Ah.. I see. So a little bit of code duplication is okay in the interest of keeping the class hierarchy more flat.
I added RelrBaseSection, which stores std::vector<RelativeReloc>.
RelativeReloc is similar to DynamicReloc, but only stores Section and Offset.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48247





More information about the llvm-commits mailing list