[PATCH] D29983: [LLD][ELF] Calculate sizes of SHF_ALLOC Synthetic Sections early

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:642-656
+template <class ELFT> void MipsGotSection<ELFT>::finalize() {
+  PageEntriesNum = 0;
+  for (std::pair<const OutputSectionBase *, size_t> &P : PageIndexMap) {
+    // For each output section referenced by GOT page relocations calculate
+    // and save into PageIndexMap an upper bound of MIPS GOT entries required
+    // to store page addresses of local symbols. We assume the worst case -
+    // each 64kb page of the output section has at least one GOT relocation
----------------
Is this computing the same value just for the last assertion? If so it seems a bit too much.


================
Comment at: ELF/SyntheticSections.cpp:880
 
-// Add remaining entries to complete .dynamic contents.
-template <class ELFT> void DynamicSection<ELFT>::finalize() {
-  if (this->Size)
-    return; // Already finalized.
-
-  this->Link = In<ELFT>::DynStrTab->OutSec->SectionIndex;
-  if (In<ELFT>::RelaDyn->OutSec->Size > 0) {
+template <class ELFT> void DynamicSection<ELFT>::applyEntries(bool Reserve) {
+  auto Apply = [this, Reserve](Entry E){
----------------
I think I prefer a more functional style than this. You could change this function to return `std::vector<Entry>` and call that twice, once in finalizeAllocSize and the other in finalize.


================
Comment at: ELF/SyntheticSections.cpp:881
+template <class ELFT> void DynamicSection<ELFT>::applyEntries(bool Reserve) {
+  auto Apply = [this, Reserve](Entry E){
+    if (Reserve)
----------------
nit: For short lambdas like this probably just `[=]` is cognitively lightweight than explicit variable lists.


================
Comment at: ELF/SyntheticSections.cpp:1120
 
-template <class ELFT> void SymbolTableSection<ELFT>::finalize() {
-  this->OutSec->Link = this->Link = StrTabSec.OutSec->SectionIndex;
-  this->OutSec->Info = this->Info = NumLocals + 1;
-  this->OutSec->Entsize = this->Entsize;
-
-  if (Config->Relocatable)
-    return;
-
-  if (!StrTabSec.isDynamic()) {
-    auto GlobBegin = Symbols.begin() + NumLocals;
-    auto It = std::stable_partition(
-        GlobBegin, Symbols.end(), [](const SymbolTableEntry &S) {
-          return S.Symbol->symbol()->computeBinding() == STB_LOCAL;
-        });
-    // update sh_info with number of Global symbols output with computed
-    // binding of STB_LOCAL
-    this->OutSec->Info = this->Info = 1 + (It - Symbols.begin());
+template <class ELFT> void SymbolTableSection<ELFT>::finalizeAllocSize() {
+  if (Config->Relocatable || !StrTabSec.isDynamic())
----------------
The reason why you need this is because if it is a dynamic symbol table, finalizing it may change (or fix) the size of a .gnu.hash section, right? Please add a comment.


================
Comment at: ELF/Writer.cpp:1222
     OS->addSection(make<ARMExidxSentinelSection<ELFT>>());
+    OS->Size += 8;
+  }
----------------
What is this?


https://reviews.llvm.org/D29983