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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/InputSection.h:43
 public:
-  enum Kind { Regular, EHFrame, Merge, Synthetic, };
+  enum Kind { Regular, EHFrame, Merge, Synthetic, SyntheticMerge };
 
----------------
ruiu wrote:
> 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)
> 
Ok. Naming things was always hard for me, thanks for suggestions, I applied it.


================
Comment at: ELF/OutputSections.cpp:135
+  if (SS && SS->Mergeable)
+    static_cast<MergeSection<ELFT> *>(SS)->setOutputSection(this);
+
----------------
ruiu wrote:
> 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.
Done.


================
Comment at: ELF/Writer.cpp:212
   Script<ELFT>::X->OutputSections = &OutputSections;
+  combineMergableSections(Symtab<ELFT>::X->Sections);
   if (ScriptConfig->HasSections) {
----------------
ruiu wrote:
> Isn't this too late? I don't see a reason why we can't do this just after `createSyntheticSections`.
Well. It not "late" but also nothing stops us to do that earlier in a single place too, did that.


================
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 (
----------------
ruiu wrote:
> What is this change?
Previously we had next code:

```
template <class ELFT>
void MergeOutputSection<ELFT>::addSection(InputSectionData *C) {
...
  this->Entsize = Sec->Entsize;
...
}
```

Now we dont as handle merge sections via regular.

Actually I wrote about that situation earlier :) Please look at patch description and my comment (and "fix") for one of first diff,
which was:
"Well, that is actually the question I wish to find answer and mentioned in description of patch.
We set sh_entsize and checked it in testcases before, when each different mergeable section was a single output.
If we can ignore it and set to 0, that makes patch a bit easier. Looking on gold output which is inconsistent,
looks that value makes no sence (if not -r elocatable)."

I leaved it "just 0" as was suggested by you, though I believe you supposed it was already "0" before, but it was not.
I think it is unclear what value to leave in the output. gold as far I remember just takes the first value by order when combines sections.
So it easy to face inconsistency for its output, what IMO proves that value makes no sense for output sections.
I believe we can put zero here, like this patch do.


https://reviews.llvm.org/D27415