[JITLink][COFF] Handle out-of-order COMDAT second symbol.

[sunho]

Summary

Handle out-of-order COMDAT second symbols. In llvm codegen, the second symbol of COMDAT sequence always follows the first symbol in the global symbol list. But, when the object file came from MSVC compiler, these can come in out of order.

[sgraenitz]

Oh, good catch! Thanks

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

521

Shouldn't this assign to PendingComdatExports[Symbol.getSectionNumber()] now?

[sunho]

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

521

good catch! it miraclely built succesfully as variable with the same name was introduced.

[sunho]

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

521

For clearness, I changed PendinComdatExport to reference so that original node will be just set None.

[lhames]

LGTM.

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

193

What's the maximum number of sections in a COFF object? Could this get large for pathological objects? (In which case a DenseMap<SectionIndex, ComdatExportRequest> might be better?)

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.h

return forEachRelocation(
         ProcessDebugSections);
   }
 
 private:
   // Pending comdat symbol export that is initiated by the first symbol of
   // COMDAT sequence.
   struct ComdatExportRequest {
     COFFSymbolIndex SymbolIndex;
-    COFFSectionIndex SectionIndex;
     jitlink::Linkage Linkage;
   };
-  Optional<ComdatExportRequest> PendingComdatExport;
+  std::vector<Optional<ComdatExportRequest>> PendingComdatExports;
 
   // This represents a pending request to create a weak external symbol with a
   // name.
   struct WeakAliasRequest {
     COFFSymbolIndex Alias;
     COFFSymbolIndex Target;
     StringRef SymbolName;
   };

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

Error COFFLinkGraphBuilder::graphifySections() {
 
   return Error::success();
 }
 
 Error COFFLinkGraphBuilder::graphifySymbols() {
   LLVM_DEBUG(dbgs() << "  Creating graph symbols...\n");
 
   SymbolSets.resize(Obj.getNumberOfSections() + 1);
+  PendingComdatExports.resize(Obj.getNumberOfSections() + 1);
   GraphSymbols.resize(Obj.getNumberOfSymbols());
 
   for (COFFSymbolIndex SymIndex = 0;
        SymIndex < static_cast<COFFSymbolIndex>(Obj.getNumberOfSymbols());
        SymIndex++) {
     Expected<object::COFFSymbolRef> Sym = Obj.getSymbol(SymIndex);
     if (!Sym)
       return Sym.takeError();
Expected<Symbol *> COFFLinkGraphBuilder::createDefinedSymbol(
   if (llvm::COFF::isReservedSectionNumber(Symbol.getSectionNumber()))
     return make_error<JITLinkError>(
         "Reserved section number used in regular symbol " +
         formatv("{0:d}", SymIndex));
 
   Block *B = getGraphBlock(Symbol.getSectionNumber());
   if (Symbol.isExternal()) {
     // This is not a comdat sequence, export the symbol as it is
-    if (!isComdatSection(Section))
+    if (!isComdatSection(Section)) {
+
       return &G->addDefinedSymbol(
           *B, Symbol.getValue(), SymbolName, 0, Linkage::Strong, Scope::Default,
           Symbol.getComplexType() == COFF::IMAGE_SYM_DTYPE_FUNCTION, false);
-    else {
-      if (!PendingComdatExport)
+    } else {
+      if (!PendingComdatExports[Symbol.getSectionNumber()])
         return make_error<JITLinkError>("No pending COMDAT export for symbol " +
                                         formatv("{0:d}", SymIndex));
-      if (PendingComdatExport->SectionIndex != Symbol.getSectionNumber())
-        return make_error<JITLinkError>(
-            "COMDAT export section number mismatch for symbol " +
-            formatv("{0:d}", SymIndex));
+
       return exportCOMDATSymbol(SymIndex, SymbolName, Symbol);
     }
   }
 
   if (Symbol.getStorageClass() == COFF::IMAGE_SYM_CLASS_STATIC) {
     const object::coff_aux_section_definition *Definition =
         Symbol.getSectionDefinition();
     if (!Definition || !isComdatSection(Section)) {
       // Handle typical static symbol
       return &G->addDefinedSymbol(
           *B, Symbol.getValue(), SymbolName, 0, Linkage::Strong, Scope::Local,
           Symbol.getComplexType() == COFF::IMAGE_SYM_DTYPE_FUNCTION, false);
     }
     if (Definition->Selection == COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
       // FIXME: don't dead strip this when parent section is alive
       return &G->addDefinedSymbol(
           *B, Symbol.getValue(), SymbolName, 0, Linkage::Strong, Scope::Local,
           Symbol.getComplexType() == COFF::IMAGE_SYM_DTYPE_FUNCTION, false);
     }
-    if (PendingComdatExport)
+    if (PendingComdatExports[Symbol.getSectionNumber()])
       return make_error<JITLinkError>(
           "COMDAT export request already exists before symbol " +
           formatv("{0:d}", SymIndex));
     return createCOMDATExportRequest(SymIndex, Symbol, Definition);
   }
   return make_error<JITLinkError>("Unsupported storage class " +
                                   formatv("{0:d}", Symbol.getStorageClass()) +
                                   " in symbol " + formatv("{0:d}", SymIndex));
case COFF::IMAGE_COMDAT_SELECT_NEWEST: {
     return make_error<JITLinkError>(
         "IMAGE_COMDAT_SELECT_NEWEST is not supported.");
   }
   default: {
     return make_error<JITLinkError>("Invalid comdat selection type: " +
                                     formatv("{0:d}", Definition->Selection));
   }
   }
-  PendingComdatExport = {SymIndex, Symbol.getSectionNumber(), L};
+  PendingComdatExports[Symbol.getSectionNumber()] = {SymIndex, L};
   return &G->addAnonymousSymbol(*B, Symbol.getValue(), Definition->Length,
                                 false, false);
 }
 
 // Process the second symbol of COMDAT sequence.
 Expected<Symbol *>
 COFFLinkGraphBuilder::exportCOMDATSymbol(COFFSymbolIndex SymIndex,
                                          StringRef SymbolName,
                                          object::COFFSymbolRef Symbol) {
+  auto PendingComdatExport = PendingComdatExports[Symbol.getSectionNumber()];
   COFFSymbolIndex TargetIndex = PendingComdatExport->SymbolIndex;
   Linkage L = PendingComdatExport->Linkage;
   jitlink::Symbol *Target = getGraphSymbol(TargetIndex);
   assert(Target && "COMDAT leaader is invalid.");
   assert((llvm::count_if(G->defined_symbols(),
                          [&](const jitlink::Symbol *Sym) {
                            return Sym->getName() == SymbolName;
                          }) == 0) &&
          "Duplicate defined symbol");
   Target->setName(SymbolName);
   Target->setLinkage(L);
   Target->setCallable(Symbol.getComplexType() ==
                       COFF::IMAGE_SYM_DTYPE_FUNCTION);
   Target->setScope(Scope::Default);
   LLVM_DEBUG({
     dbgs() << "    " << SymIndex
            << ": Exporting COMDAT graph symbol for COFF symbol \"" << SymbolName
            << "\" in section " << Symbol.getSectionNumber() << "\n";
     dbgs() << "      " << *Target << "\n";
   });
   PendingComdatExport = None;
   return Target;
 }
 
 } // namespace jitlink
 } // namespace llvm
 No newline at end of file

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_intervene.test

+# REQUIRES: asserts
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-jitlink -noexec --debug-only=jitlink -noexec %t 2>&1 | FileCheck %s
+# 
+# Check a comdat export is done correctly even if second symbol of comdat sequences appear out of order
+#
+# CHECK: Creating graph symbols...
+# CHECK:      6: Exporting COMDAT graph symbol for COFF symbol "func2" in section 3
+# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000001, linkage: weak, scope: default, dead  -   func2
+# CHECK:      7: Exporting COMDAT graph symbol for COFF symbol "func" in section 2
+# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000001, linkage: weak, scope: default, dead  -   func
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          1
+  - Name:            .text
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          2
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            .text
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          3
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            func2
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            func
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...