[PATCH] D45801: COFF: Use (name, output characteristics) as a key when grouping input sections into output sections.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D45801#1072340, @zturner wrote:

> In https://reviews.llvm.org/D45801#1072314, @rnk wrote:
>
> > @zturner: We need to add logic to this section merging code to make S_SECTION / S_COFFGROUP records.
>
>
> We already emit the `S_SECTION` symbols (see the function `addLinkerModuleSectionSymbol`), but you're right, we don't emit the `S_COFFGROUP` symbols yet.


What is an `S_COFFGROUP`? From looking at our test cases it appears to be a group of input sections with the same name (including the `$` suffix), right? Can't we infer them in the PDB writer by looking at input section names?



================
Comment at: lld/COFF/Writer.cpp:719
+    }
+    To->merge(From);
+  }
----------------
rnk wrote:
> Do we need to remove `From` somehow? What are the semantics of `/merge:.foo=.bar /merge:.baz=.foo`? Move all input sections in .foo to .bar, and then move all input sections from .baz to .foo? Or, everything in .bar?
We don't need to remove `From` because it will be taken care of by `removeEmptySections`.

The semantics for your example are that everything gets moved to `.bar`, but before this change we were just taking one step (so `.baz` is renamed to `.foo` and the input `.foo` sections are merged into `.bar`). I didn't intend to change this as part of this change, but it turns out that we happen to do the right thing in your example because `.baz` is alphabetically less than `.foo`. Probably the best thing to do is to implement multi-step merging in this change as well.


================
Comment at: lld/COFF/Writer.cpp:1127-1129
+    for (OutputSection *Sec : OutputSections)
+      if (Sec->Name == Name)
+        Sec->setPermissions(Perm);
----------------
ruiu wrote:
> Does this mean that we now have multiple sections that have the same name? Is it OK?
It seems to be fine, it's what link.exe does and the Windows loader doesn't have a problem with it.


https://reviews.llvm.org/D45801