[PATCH] D49700: [LLD] [COFF] Treat .xdata/.pdata$<sym> as implicitly associative to <sym> for MinGW

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/InputFiles.cpp:263
 
+  std::map<StringRef, uint32_t> SymbolSectionIndexes;
   std::vector<const coff_aux_section_definition *> ComdatDefs(
----------------
mstorsjo wrote:
> rnk wrote:
> > Is there some way we can hash fewer symbols? It's unfortunate that we need a string hash table at all to parse object files. I wonder if there's a way we can defer this work, since for C++, a lot of comdat things end up getting discarded.
> > 
> > If we do need a string hash table, it should probably be a `DenseMap<StringRef, uint32_t>`, since this is performance critical.
> > 
> > Oh, here's an idea: maybe `createDefined` should return `Prevailing` somehow. Only add the symbol to the table if it's a prevailing comdat symbol in an executable section.
> Hmm, I'll try, unless the other suggestion turns out to be enough.
I tried implementing this to reduce the amount of hashing, in case we still want to try to do this (for GCC object compatibility or if we don't want to do the other patch anyway).


================
Comment at: test/COFF/associative-comdat-mingw.s:40-43
+        .section        .xdata$foo,"dr",associative,foo
+#        .linkonce       discard
+        .p2align        3
+        .long           42
----------------
mstorsjo wrote:
> rnk wrote:
> > I wonder if we could keep using associative comdats for .xdata and .pdata, but use the GCC-style section names. I wonder if ld.bfd would do the right thing with them. That would allow us to avoid the string hash table and save us a fair amount of link speed.
> Good idea, I guess it probably would work, but I'll check.
> 
> That won't help with object files from GCC though, but GCC+LLD isn't a priority to anyone afaik, and I there's probably other bigger issues in that setup anyway.
I tested this, and this compromise does seem to work - I uploaded a patch for this version at https://reviews.llvm.org/D49757.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49700