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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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;
----------------
Can this just be a boolean flag AndroidPackDynRelocs?


================
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
----------------
I first thought that "*" is to emphasize a word and a closing "*" were missing.


================
Comment at: lld/ELF/SyntheticSections.cpp:1357
+  //   this relocation.
+
+  size_t OldSize = RelocData.size();
----------------
I'd also mention that how many bytes could be saved by this encoding compared to the default dynamic relocation table to give an insight why this format was invented and is used.


================
Comment at: lld/ELF/SyntheticSections.cpp:1379
+
+    if (R.getType(Config->IsMips64EL) == Target->RelativeRel)
+      Relatives.push_back(R);
----------------
I believe you can use `Rel.Type` insetad of `R.getType(Config->IsMips64EL)`.


================
Comment at: lld/ELF/SyntheticSections.cpp:1486
+
+  return RelocData.size() != OldSize;
 }
----------------
This needs comment.


================
Comment at: lld/ELF/SyntheticSections.h:415
+  void writeTo(uint8_t *Buf) override {
+    std::copy(RelocData.begin(), RelocData.end(), Buf);
+  }
----------------
std::copy should work, but for consistency with other `writeTo` functions, memcpy would be better.


================
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"
----------------
What's wrong with doing this for packed relocations?


https://reviews.llvm.org/D39152