[PATCH] D39152: ELF: Add support for emitting dynamic relocations in the Android relocation packing format.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lld/ELF/Config.h:167
   ELFKind EKind = ELFNoneKind;
+  PackDynRelocsFormat PackDynRelocs = PackDynRelocsFormat::None;
   uint16_t DefaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
----------------
ruiu wrote:
> Can this just be a boolean flag AndroidPackDynRelocs?
It can for now, I suppose.


================
Comment at: lld/ELF/SyntheticSections.cpp:1245
+void RelocationBaseSection::finalizeContents() {
+  // If all relocations are *RELATIVE they don't refer to any
+  // dynamic symbol and we don't need a dynamic symbol table. If that
----------------
ruiu wrote:
> I first thought that "*" is to emphasize a word and a closing "*" were missing.
Note that this is just moving code around (see line 1287 in the old code). But seems reasonable to change it to `R_*_RELATIVE`.


================
Comment at: lld/ELF/SyntheticSections.cpp:1379
+
+    if (R.getType(Config->IsMips64EL) == Target->RelativeRel)
+      Relatives.push_back(R);
----------------
ruiu wrote:
> I believe you can use `Rel.Type` insetad of `R.getType(Config->IsMips64EL)`.
I don't think that will work because the type is stored in the `r_info` field.


================
Comment at: lld/ELF/Writer.cpp:373-374
   In<ELFT>::RelaIplt = make<RelocationSection<ELFT>>(
-      (Config->EMachine == EM_ARM) ? ".rel.dyn" : In<ELFT>::RelaPlt->Name,
+      (Config->EMachine == EM_ARM &&
+       Config->PackDynRelocs == PackDynRelocsFormat::None)
+          ? ".rel.dyn"
----------------
ruiu wrote:
> What's wrong with doing this for packed relocations?
That would cause a section type mismatch. I've added a comment with a few more details.


https://reviews.llvm.org/D39152