[LLD] [COFF] Check the aux section definition size for IMAGE_COMDAT_SELECT_SAME_SIZE

[mstorsjo]

Summary

Binutils generated sections seem to be padded to a multiple of 16 bytes, but the aux section definition contains the original, unpadded section length.

The size check used for IMAGE_COMDAT_SELECT_SAME_SIZE previously only checked the size of the section itself. When checking the currently processed object file against the previously chosen comdat section, we easily have access to the aux section definition of the currently processed section, but the previously selected (from a different object file) isn't currently available, as far as I can see.

This patch adds the length as a member to SectionChunk, which is a frequently allocated class, and effort has been spent to minimize its size - so this is a step backwards in that regard.

This fixes statically linking clang-built C++ object files against libstdc++ built with GCC, if the object files contain e.g. typeinfo.

I'm tagging Reid here, who has spent effort on minimizing SectionChunk even though I know he's on leave and probably won't comment.

Are there other suggestions on how to achieve the same, without inflating SectionChunk?

We do have access to the SectionChunk for the leader, which gives the ObjFile that defined it, but we'd have to iterate over all symbols in that COFFObjectFile to find the coff_aux_section_definition that matches the SectionChunk. But maybe that's tolerable for the edge case of comdat collisions with IMAGE_COMDAT_SELECT_SAME_SIZE where the section sizes mismatch?

[mstorsjo]

Updated to not change SectionChunk and instead iterate through the leader's object file, looking for the right coff_aux_section_definition, in the few cases where the size is needed.

[amccarth]

Not my area of expertise, but this looks good to me. Before submitting, please make sure you update the commit description since you didn't end up adding any fields to SectionChunk.

[amccarth]

Oops, I forgot to "Accept Revision."

lld/COFF/InputFiles.h

void maybeAssociateSEHForMingw(
       const llvm::DenseMap<StringRef, uint32_t> &prevailingSectionMap);
 
   // Given a new symbol Sym with comdat selection Selection, if the new
   // symbol is not (yet) Prevailing and the existing comdat leader set to
   // Leader, emits a diagnostic if the new symbol and its selection doesn't
   // match the existing symbol and its selection. If either old or new
   // symbol have selection IMAGE_COMDAT_SELECT_LARGEST, Sym might replace
   // the existing leader. In that case, Prevailing is set to true.
-  void handleComdatSelection(COFFSymbolRef sym,
-                             llvm::COFF::COMDATType &selection,
-                             bool &prevailing, DefinedRegular *leader);
+  void
+  handleComdatSelection(COFFSymbolRef sym, llvm::COFF::COMDATType &selection,
+                        bool &prevailing, DefinedRegular *leader,
+                        const llvm::object::coff_aux_section_definition *def);
 
   llvm::Optional<Symbol *>
   createDefined(COFFSymbolRef sym,
                 std::vector<const llvm::object::coff_aux_section_definition *>
                     &comdatDefs,
                 bool &prevailingComdat);
   Symbol *createRegular(COFFSymbolRef sym);
   Symbol *createUndefined(COFFSymbolRef sym);

lld/COFF/InputFiles.cpp

void ObjFile::initializeSymbols() {
   decltype(sparseChunks)().swap(sparseChunks);
 }
 
 Symbol *ObjFile::createUndefined(COFFSymbolRef sym) {
   StringRef name = check(coffObj->getSymbolName(sym));
   return symtab->addUndefined(name, this, sym.isWeakExternal());
 }
 
-void ObjFile::handleComdatSelection(COFFSymbolRef sym, COMDATType &selection,
-                                    bool &prevailing, DefinedRegular *leader) {
+static const coff_aux_section_definition *findSectionDef(COFFObjectFile *obj,
+                                                         int32_t section) {
+  uint32_t numSymbols = obj->getNumberOfSymbols();
+  for (uint32_t i = 0; i < numSymbols; ++i) {
+    COFFSymbolRef sym = check(obj->getSymbol(i));
+    if (sym.getSectionNumber() != section)
+      continue;
+    if (const coff_aux_section_definition *def = sym.getSectionDefinition())
+      return def;
+  }
+  return nullptr;
+}
+
+void ObjFile::handleComdatSelection(
+    COFFSymbolRef sym, COMDATType &selection, bool &prevailing,
+    DefinedRegular *leader,
+    const llvm::object::coff_aux_section_definition *def) {
   if (prevailing)
     return;
   // There's already an existing comdat for this symbol: `Leader`.
   // Use the comdats's selection field to determine if the new
   // symbol in `Sym` should be discarded, produce a duplicate symbol
   // error, etc.
 
   SectionChunk *leaderChunk = nullptr;
case IMAGE_COMDAT_SELECT_NODUPLICATES:
     symtab->reportDuplicate(leader, this);
     break;
 
   case IMAGE_COMDAT_SELECT_ANY:
     // Nothing to do.
     break;
 
   case IMAGE_COMDAT_SELECT_SAME_SIZE:
-    if (leaderChunk->getSize() != getSection(sym)->SizeOfRawData)
+    if (leaderChunk->getSize() != getSection(sym)->SizeOfRawData) {
+      if (!config->mingw) {
+        symtab->reportDuplicate(leader, this);
+      } else {
+        const coff_aux_section_definition *leaderDef = findSectionDef(
+            leaderChunk->file->getCOFFObj(), leaderChunk->getSectionNumber());
+        if (!leaderDef || leaderDef->Length != def->Length)
           symtab->reportDuplicate(leader, this);
+      }
+    }
     break;
 
   case IMAGE_COMDAT_SELECT_EXACT_MATCH: {
     SectionChunk newChunk(this, getSection(sym));
     // link.exe only compares section contents here and doesn't complain
     // if the two comdat sections have e.g. different alignment.
     // Match that.
     if (leaderChunk->getContents() != newChunk.getContents())
if (def->Selection < (int)IMAGE_COMDAT_SELECT_NODUPLICATES ||
         // doesn't understand IMAGE_COMDAT_SELECT_NEWEST either.
         def->Selection > (int)IMAGE_COMDAT_SELECT_LARGEST) {
       fatal("unknown comdat type " + std::to_string((int)def->Selection) +
             " for " + getName() + " in " + toString(this));
     }
     COMDATType selection = (COMDATType)def->Selection;
 
     if (leader->isCOMDAT)
-      handleComdatSelection(sym, selection, prevailing, leader);
+      handleComdatSelection(sym, selection, prevailing, leader, def);
 
     if (prevailing) {
       SectionChunk *c = readSection(sectionNumber, def, getName());
       sparseChunks[sectionNumber] = c;
       c->sym = cast<DefinedRegular>(leader);
       c->selection = selection;
       cast<DefinedRegular>(leader)->data = &c->repl;
     } else {

lld/test/COFF/Inputs/comdat-binutils.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_RELOCS_STRIPPED, IMAGE_FILE_LINE_NUMS_STRIPPED ]
+sections:
+  - Name:            '.rdata$mysymbol'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     2A000000000000000000000000000000
+symbols:
+  - Name:            '.rdata$mysymbol'
+    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:        0
+      Number:          0
+      Selection:       IMAGE_COMDAT_SELECT_SAME_SIZE
+  - Name:            mysymbol
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

lld/test/COFF/Inputs/comdat-llvm.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            '.rdata$mysymbol'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ ]
+    Alignment:       1
+    SectionData:     2A
+symbols:
+  - Name:            '.rdata$mysymbol'
+    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:        3686517206
+      Number:          1
+      Selection:       IMAGE_COMDAT_SELECT_SAME_SIZE
+  - Name:            mysymbol
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

lld/test/COFF/comdat-gcc-compatibility-size.test

+# RUN: yaml2obj %p/Inputs/comdat-llvm.yaml > %t.llvm.o
+# RUN: yaml2obj %p/Inputs/comdat-binutils.yaml > %t.binutils.o
+# RUN: lld-link -lldmingw -noentry -dll %t.llvm.o %t.binutils.o -out:%t.dll
+# RUN: lld-link -lldmingw -noentry -dll %t.binutils.o %t.llvm.o -out:%t.dll
+# RUN: not lld-link -noentry -dll %t.llvm.o %t.binutils.o -out:%t.dll
+# RUN: not lld-link -noentry -dll %t.binutils.o %t.llvm.o -out:%t.dll
+
+# The test object files have been generated by assembling the following
+# snippet using binutils and llvm.
+
+#        .section .rdata$mysymbol, "dr"
+#        .linkonce same_size
+#        .globl mysymbol
+#mysymbol:
+#        .byte 42