[PATCH] D55293: [LLD][COFF] Unmerged sections

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked 2 inline comments as done.
aganea added inline comments.


================
Comment at: COFF/Writer.cpp:695
 
-  SmallDenseMap<std::pair<StringRef, uint32_t>, OutputSection *> Sections;
-  auto CreateSection = [&](StringRef Name, uint32_t OutChars) {
-    OutputSection *&Sec = Sections[{Name, OutChars}];
+  std::map<NameAndChars, OutputSection *> OutSectionsMap;
+  auto CreateOutSection = [&](StringRef Name, uint32_t OutChars) {
----------------
rnk wrote:
> Eh, this is small, since it's only the real output sections of the PE. I guess it doesn't matter if we use std::map for simplicity.
This is essentially due to the new `NameAndChars` structure. If we want to use a (Small)DenseMap here, it needs a bit of boilerplate for `DenseMapInfo<NameAndChars>`, only to be used here. I don't think space or perf. will be an issue here.


================
Comment at: COFF/Writer.h:82
 
+  std::vector<UnmergedSection *> ContribSections;
+
----------------
ruiu wrote:
> ruiu wrote:
> > mstorsjo wrote:
> > > Maybe leave out the `ContribSections` part from this patch - as far as I see it's not actually needed/used at this stage yet, and introducing it here makes the patch a bit more confusing.
> > I don't think I understand the point of this change. Should the fact that some section is not assigned to some output section be a C++ class? It seems to me that that's a bit weird concept to be a class. Also, OutputSection has a list of unmerged sections, which seems odd to me too because once they are added, they are no longer unmerged.
> > 
> > I guess that first of all, I don't understand the intention of this change. Are you changing the code so that you can handle sections in input files and synthetic sections in the same way?
> It seems you are not using this member.
You are right, this member shouldn't be part of the current patch, I will remove it.
@ruiu The overall intention of this patch is to support Coff Groups, as proposed in D54802.
```
Mod 0003 | `* Linker *`:
[...]
   588 | S_SECTION [size = 28] `.text`
         length = 38, alignment = 12, rva = 4096, section # = 1
         characteristics =
           code
           execute permissions
           read permissions
   616 | S_COFFGROUP [size = 24] `.text`
         length = 8, addr = 0001:0000
         characteristics =
           code
           execute permissions
           read permissions
   640 | S_SECTION [size = 28] `.rdata`
         length = 61, alignment = 12, rva = 8192, section # = 2
         characteristics =
           initialized data
           read permissions
   668 | S_SECTION [size = 28] `.idata`
         length = 145, alignment = 12, rva = 12288, section # = 3
         characteristics =
           initialized data
           read permissions
   696 | S_COFFGROUP [size = 28] `.idata$2`
         length = 40, addr = 0003:0000
         characteristics =
           initialized data
           read permissions
           write permissions
   724 | S_COFFGROUP [size = 28] `.idata$4`
         length = 24, addr = 0003:0040
         characteristics =
           initialized data
           read permissions
           write permissions
   752 | S_COFFGROUP [size = 28] `.idata$5`
         length = 24, addr = 0003:0064
         characteristics =
           initialized data
           read permissions
           write permissions
   780 | S_COFFGROUP [size = 28] `.idata$6`
         length = 24, addr = 0003:0088
         characteristics =
           initialized data
           read permissions
           write permissions
   808 | S_COFFGROUP [size = 28] `.idata$7`
         length = 33, addr = 0003:0112
         characteristics =
           initialized data
           read permissions
           write permissions
```
Without this, the unmerged sections are otherwise lost in `Writer::mergeSections()`, and are not available later in `PDBLinker::addSections()`.



Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55293/new/

https://reviews.llvm.org/D55293