[PATCH] D56929: lld-link: Use just one code path to process associative comdats, reject some invalid associated comdats

[Nico Weber via Phabricator via llvm-commits]

thakis created this revision.
thakis added reviewers: pcc, ruiu.

Currently, if an associative comdat appears after the comdat it's associated with it's processed immediately, else it's deferred until the end of the object file. I found this confusing to think about while working on PR40094, so this patch makes it so that associated comdats are always processed at the end of the object file. Tests seem happy, are there problems with this?

If not: Now there's a natural place to reject the associated comdats referring to later associated comdats (associated comdats referring to associated comdats is invalid per COFF spec) that , so reject those. (A later patch will reject associated comdats referring to earlier comdats.)


https://reviews.llvm.org/D56929

Files:
  lld/COFF/InputFiles.cpp


Index: lld/COFF/InputFiles.cpp
===================================================================
--- lld/COFF/InputFiles.cpp
+++ lld/COFF/InputFiles.cpp
@@ -227,11 +227,17 @@
                                         uint32_t ParentSection) {
   SectionChunk *Parent = SparseChunks[ParentSection];
 
-  // If the parent is pending, it probably means that its section definition
-  // appears after us in the symbol table. Leave the associated section as
-  // pending; we will handle it during the second pass in initializeSymbols().
-  if (Parent == PendingComdat)
+  if (Parent == PendingComdat) {
+    // This can only happen if an associative comdat refers to another
+    // associative comdat that appears after it.
+    StringRef Name, ParentName;
+    COFFObj->getSymbolName(Sym, Name);
+    COFFSymbolRef ParentSym = check(COFFObj->getSymbol(ParentSection));
+    COFFObj->getSymbolName(ParentSym, ParentName);
+    error("associative comdat " + Name +
+          " refers to associative comdat " + ParentName);
     return;
+  }
 
   // Check whether the parent is prevailing. If it is, so are we, and we read
   // the section; otherwise mark it as discarded.
@@ -438,21 +444,15 @@
     return Leader;
   }
 
-  // Read associative section definitions and prepare to handle the comdat
-  // leader symbol by setting the section's ComdatDefs pointer if we encounter a
-  // non-associative comdat.
+  // Prepare to handle the comdat leader symbol by setting the section's
+  // ComdatDefs pointer if we encounter a non-associative comdat.
   if (SparseChunks[SectionNumber] == PendingComdat) {
     if (const coff_aux_section_definition *Def = Sym.getSectionDefinition()) {
-      if (Def->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE)
-        readAssociativeDefinition(Sym, Def);
-      else
+      if (Def->Selection != IMAGE_COMDAT_SELECT_ASSOCIATIVE)
         ComdatDefs[SectionNumber] = Def;
     }
-  }
-
-  // readAssociativeDefinition() writes to SparseChunks, so need to check again.
-  if (SparseChunks[SectionNumber] == PendingComdat)
     return None;
+  }
 
   return createRegular(Sym);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56929.182583.patch
Type: text/x-patch
Size: 2116 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190118/dcc2f7f2/attachment.bin>