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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

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



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


================
Comment at: COFF/InputFiles.cpp:271-272
       WeakAliases.emplace_back(Symbols[I], TagIndex);
-    } else if (Optional<Symbol *> OptSym = createDefined(COFFSym, ComdatDefs)) {
+    } else if (Optional<Symbol *> OptSym =
+                   createDefined(COFFSym, ComdatDefs, PrevailingComdat)) {
       Symbols[I] = *OptSym;
----------------
As future work, I think createDefined is doing too much work. Ideally, we should find a way to avoid the Optional<> return value.


================
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];
----------------
Can you pull this out into a method like `recordPrevailingSymbolForMingw`?


================
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())
----------------
And pull this out into `maybeAssociateSEHForMingw`?


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


https://reviews.llvm.org/D49700