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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D55293#1328930 <https://reviews.llvm.org/D55293#1328930>, @rnk wrote:

> Yep, I looked at removing this before. It might actually be worth it for mingw, because GCC outputs sections named like ".text$_Z3foov", so you can imagine that this map is going to have one entry per symbol, so one extra memory allocation per symbol.
>
> @mstorsjo, I assume that ld.bfd doesn't actually sort the .text (and .pdata and .xdata) section contributions alphabetically. Should we drop the "$_Z3foov" portion of the object file section name before making the unmerged section? It might matter for performance.


ld.bfd does sort the section contributions alphabetically. I tested with a test snippet like this:

  $ cat test.cpp
  inline int foo1() {
      return 1;
  }
  inline int foo3() {
      return 3;
  }
  inline int foo2() {
      return 2;
  }
  int main(int argc, char* argv[]) {
      return foo1() + foo3() + foo2();
  }
  $ x86_64-w64-mingw32-g++ test.cpp -c
  $ x86_64-w64-mingw32-g++ test.o -o test.exe -Wl,-Map,map.txt
  $ cat map.txt
  ...
   .text          0x0000000000402da0        0x0 /Users/martin/mingw64/lib/gcc/x86_64-w64-mingw32/5.4.0/crtend.o
   *(SORT(.text$*))
   .text$_Z4foo1v
                  0x0000000000402da0       0x10 test.o
                  0x0000000000402da0                foo1()
   .text$_Z4foo2v
                  0x0000000000402db0       0x10 test.o
                  0x0000000000402db0                foo2()
   .text$_Z4foo3v
                  0x0000000000402dc0       0x10 test.o
                  0x0000000000402dc0                foo3()

So it does seem to be sorted, intentionally.



================
Comment at: COFF/Writer.cpp:212
   void sortCRTSectionChunks(std::vector<Chunk *> &Chunks);
+  void addSyntheticIdata(IdataContents &Idata);
+  bool fixGnuImportChunks();
----------------
I think the parameter is unnecessary when converting this from a static function to a method, as the class already has got the same `IdataContents Idata` member.


================
Comment at: COFF/Writer.cpp:669
   uint32_t RDATA = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ;
-  std::vector<Chunk *> &ImportTables = Map[{".idata$2", RDATA}];
-  if (!ImportTables.empty())
-    ImportTableStart = ImportTables.front();
-  for (Chunk *C : ImportTables)
-    ImportTableSize += C->getSize();
-
-  std::vector<Chunk *> &IAT = Map[{".idata$5", RDATA}];
-  if (!IAT.empty())
-    IATStart = IAT.front();
-  for (Chunk *C : IAT)
-    IATSize += C->getSize();
+  if (UnmergedSection *ImportSec = UnmergedSections[{".idata$2", RDATA}]) {
+    if (!ImportSec->Chunks.empty())
----------------
I'd maybe name it `ImportTableSec` or so - everything in .idata is relating to imports so `ImportSec` is a bit generic as it would go for all of them. This one specifically refers to the sections that `ImportTableStart`/`ImportTableSize` refers to.


================
Comment at: COFF/Writer.h:82
 
+  std::vector<UnmergedSection *> ContribSections;
+
----------------
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.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D55293