[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.h:43
 public:
-  enum Kind { Regular, EHFrame, Merge, Synthetic, };
+  enum Kind { Regular, EHFrame, Merge, Synthetic, SyntheticMerge };
 
----------------
The naming of native merge section and synthetic merge section is confusing. Here, you name synthetic merge section SyntheticMerge, but you name an instance of it MergeSection at other place. On the other hand, the section named Merge here has an instance of MergeInputSection. Naming needs to be consistent. Here is my suggestion.

  Kinds:
  enum Kind { Regular, EHFrame, Merge, Synthetic, MergeSynthetic };

  Classes:
  MergeInputSection (native)
  MergeSyntheticSection (synthetic)



================
Comment at: ELF/OutputSections.cpp:135
+  if (SS && SS->Mergeable)
+    static_cast<MergeSection<ELFT> *>(SS)->setOutputSection(this);
+
----------------
grimar wrote:
> ruiu wrote:
> > You seems to be reinventing dynamic class dispatching. Please define dyn_cast<> for MergeSection<ELFT>.
> Done.
You don't need a static_cast.


================
Comment at: ELF/Writer.cpp:212
   Script<ELFT>::X->OutputSections = &OutputSections;
+  combineMergableSections(Symtab<ELFT>::X->Sections);
   if (ScriptConfig->HasSections) {
----------------
Isn't this too late? I don't see a reason why we can't do this just after `createSyntheticSections`.


================
Comment at: test/ELF/gc-merge-local-sym.s:18
 // CHECK-NEXT: AddressAlignment: 1
-// CHECK-NEXT: EntrySize: 1
+// CHECK-NEXT: EntrySize: 0
 // CHECK-NEXT: SectionData (
----------------
What is this change?


https://reviews.llvm.org/D27415