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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D49700#1189936, @rnk wrote:

> I've made some suggestions to improve readability. Hopefully I'm channeling @ruiu well enough to land this after implementing them.


Thanks, it sure looks a lot more palatable after these changes!



================
Comment at: COFF/InputFiles.cpp:258
 
+  DenseMap<StringRef, uint32_t> SymbolSectionIndexes;
   std::vector<const coff_aux_section_definition *> ComdatDefs(
----------------
rnk wrote:
> Let's name this `PrevailingSectionMap` or something like that. Basically, it's a map from prevailing comdat symbols to their section number.
Done


================
Comment at: COFF/InputFiles.cpp:274-285
+      if (Config->MinGW && PrevailingComdat) {
+        // For comdat symbols in executable sections, where this is the copy
+        // of the section chunk we actually include instead of discarding it,
+        // add the symbol to a map to allow using it for implicitly
+        // associating .[px]data$<func> sections to it.
+        int32_t SectionNumber = COFFSym.getSectionNumber();
+        SectionChunk *SC = SparseChunks[SectionNumber];
----------------
rnk wrote:
> Can you pull this out into a method like `recordPrevailingSymbolForMingw`?
Done


================
Comment at: COFF/InputFiles.cpp:308-316
+        StringRef Name;
+        COFFObj->getSymbolName(Sym, Name);
+        if (Name.consume_front(".pdata$") || Name.consume_front(".xdata$")) {
+          // For MinGW, treat .[px]data$<func> as implicitly associative to
+          // the symbol <func>.
+          auto ParentSym = SymbolSectionIndexes.find(Name);
+          if (ParentSym != SymbolSectionIndexes.end())
----------------
rnk wrote:
> And pull this out into `maybeAssociateSEHForMingw`?
Done


================
Comment at: COFF/InputFiles.cpp:392
     Symbol *Leader;
     bool Prevailing;
     if (Sym.isExternal()) {
----------------
rnk wrote:
> IMO it would be simpler to eliminate this and use PrevailingComdat in std::tie below. Maybe name the parameter `Prevailing` to match.
Done


https://reviews.llvm.org/D49700