[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/Chunks.h

public:
 
   // The COMDAT leader symbol if this is a COMDAT chunk.
   DefinedRegular *sym = nullptr;
 
   // The CRC of the contents as described in the COFF spec 4.5.5.
   // Auxiliary Format 5: Section Definitions. Used for ICF.
   uint32_t checksum = 0;
 
+  // The section chunk length as defined in the section aux definition,
+  // if available.
+  uint32_t sectionDefLength = 0;
+
   // Used by the garbage collector.
   bool live;
 
   // Whether this section needs to be kept distinct from other sections during
   // ICF. This is set by the driver using address-significance tables.
   bool keepUnique = false;
 
   // The COMDAT selection if this is a COMDAT chunk.

lld/COFF/Chunks.cpp

SectionChunk::SectionChunk(ObjFile *f, const coff_section *h)
   // files will be built with -ffunction-sections or /Gy, so most things worth
   // stripping will be in a comdat.
   live = !config->doGC || !isCOMDAT();
 }
 
 // SectionChunk is one of the most frequently allocated classes, so it is
 // important to keep it as compact as possible. As of this writing, the number
 // below is the size of this class on x64 platforms.
-static_assert(sizeof(SectionChunk) <= 88, "SectionChunk grew unexpectedly");
+static_assert(sizeof(SectionChunk) <= 96, "SectionChunk grew unexpectedly");
 
 static void add16(uint8_t *p, int16_t v) { write16le(p, read16le(p) + v); }
 static void add32(uint8_t *p, int32_t v) { write32le(p, read32le(p) + v); }
 static void add64(uint8_t *p, int64_t v) { write64le(p, read64le(p) + v); }
 static void or16(uint8_t *p, uint16_t v) { write16le(p, read16le(p) | v); }
 static void or32(uint8_t *p, uint32_t v) { write32le(p, read32le(p) | v); }
 
 // Verify that given sections are appropriate targets for SECREL

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

SectionChunk *ObjFile::readSection(uint32_t sectionNumber,
 
   // Ignore DWARF debug info unless /debug is given.
   if (!config->debug && name.startswith(".debug_"))
     return nullptr;
 
   if (sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_REMOVE)
     return nullptr;
   auto *c = make<SectionChunk>(this, sec);
-  if (def)
+  if (def) {
     c->checksum = def->CheckSum;
+    c->sectionDefLength = def->Length;
+  }
 
   // CodeView sections are stored to a different vector because they are not
   // linked in the regular manner.
   if (c->isCodeView())
     debugChunks.push_back(c);
   else if (name == ".gfids$y")
     guardFidChunks.push_back(c);
   else if (name == ".gljmp$y")
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) {
+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 || leaderChunk->sectionDefLength != 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-compatibility2.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