[PATCH] D27415: [ELF] - Replace MergeOutputSection with synthetic input section MergeSection.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputSection.cpp:88
+  if (!Config->Relocatable && !isa<MergeInputSection<ELFT>>(this) &&
+      !isa<MergeSection<ELFT>>(this))
     this->Flags &= ~(SHF_MERGE | SHF_STRINGS);
----------------
How does this new condition happen?


================
Comment at: ELF/InputSection.cpp:135-136
+  case Merge: {
+    const MergeInputSection<ELFT> *MS = cast<MergeInputSection<ELFT>>(this);
+    return MS->MergeSec ? MS->MergeSec->OutSecOff + MS->getOffset(Offset) : 0;
+  }
----------------
Is there a reason to return 0? If you shouldn't call this function until an output section is set, you want to add an assert instead of returning a fake value.


================
Comment at: ELF/OutputSections.cpp:139
+
+  // If we add mergable input section we might want to create or
+  // use existing synthetic MergeSection for each input section group.
----------------
This is not a right place to add this code. You want to do outside of this function. Instead of hiding this logic inside `addSection`, make a new function that scans `Symtab<ELFT>::X->Sections` vector to merge mergeable input sections into synthetic mergeable sections.


================
Comment at: ELF/OutputSections.h:75
+  // not hold a table of fixed-size entries.
+  void updateEntsize(uint64_t Size) {
+    if (!Size || Entsize == (uint64_t)-1)
----------------
I do not understand why we need to care about sh_entsize of our output files. Why can't you just leave it 0?


================
Comment at: ELF/SyntheticSections.cpp:1691-1696
+  if (Finalized)
+    return;
+  if (shouldTailMerge())
+    finalizeTailMerge();
+  else
+    finalizeNoTailMerge();
----------------
nit: add a blank line between different code blocks. At first sight this looked like `if ~ else if ~ else`.

  if (Finalize)
    return;
  Finalized = true;

  if (should...


https://reviews.llvm.org/D27415