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

[thakis]

Summary

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.)

[thakis]

forgot to add test

[thakis]

lld/COFF/InputFiles.cpp

235 ↗

(On Diff #182585)

Oh, this is probably wrong. ParentSection is a section index, not a symbol index. One sec...

[thakis]

fix diag

[pcc]

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?

Nope, it was a (premature, as it turns out) optimization. According to my benchmarking, this patch is performance neutral linking chrome_child.dll.

lld/COFF/InputFiles.cpp

239 ↗

(On Diff #182594)

I think this is not the only reason why we might end up here. We could also get here if an IMAGE_SCN_LNK_COMDAT section does not have any symbols at all (or just has a section definition) and another section is associative with it. I reckon it's probably not worth trying to disambiguate these cases in code so maybe the diagnostic should just say invalid COFF file? (I am slightly inclined to say that we shouldn't try to diagnose at all here and should just let the linker crash.)

339 ↗

(On Diff #182594)

Comment needs to be updated.

[ruiu]

lld/COFF/InputFiles.cpp

239 ↗

(On Diff #182594)

The error message is more user-friendly if you include the name of a file. You can do that by appending toString(this) to the error message.

[thakis]

tweak diagnostic, add another test

[ruiu]

LGTM

[thakis]

update comment

lld/trunk/COFF/InputFiles.cpp

 
 void ObjFile::readAssociativeDefinition(
     COFFSymbolRef Sym, const coff_aux_section_definition *Def) {
   readAssociativeDefinition(Sym, Def, Def->getNumber(Sym.isBigObj()));
 }
 
 void ObjFile::readAssociativeDefinition(COFFSymbolRef Sym,
                                         const coff_aux_section_definition *Def,
-                                        uint32_t ParentSection) {
-  SectionChunk *Parent = SparseChunks[ParentSection];
+                                        uint32_t ParentIndex) {
+  SectionChunk *Parent = SparseChunks[ParentIndex];
 
-  // 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 happen if an associative comdat refers to another associative
+    // comdat that appears after it (invalid per COFF spec) or to a section
+    // without any symbols.
+    StringRef Name, ParentName;
+    COFFObj->getSymbolName(Sym, Name);
+
+    const coff_section *ParentSec;
+    COFFObj->getSection(ParentIndex, ParentSec);
+    COFFObj->getSectionName(ParentSec, ParentName);
+    error(toString(this) + ": associative comdat " + Name +
+          " has invalid reference to section " + ParentName);
     return;
+  }
 
   // Check whether the parent is prevailing. If it is, so are we, and we read
   // the section; otherwise mark it as discarded.
   int32_t SectionNumber = Sym.getSectionNumber();
   if (Parent) {
     SparseChunks[SectionNumber] = readSection(SectionNumber, Def, "");
     if (SparseChunks[SectionNumber])
       Parent->addAssociative(SparseChunks[SectionNumber]);
if (COFFSym.isUndefined()) {
       Symbols[I] = *OptSym;
       if (Config->MinGW && PrevailingComdat)
         recordPrevailingSymbolForMingw(COFFSym, PrevailingSectionMap);
     } else {
       // createDefined() returns None if a symbol belongs to a section that
       // was pending at the point when the symbol was read. This can happen in
       // two cases:
       // 1) section definition symbol for a comdat leader;
-      // 2) symbol belongs to a comdat section associated with a section whose
-      //    section definition symbol appears later in the symbol table.
+      // 2) symbol belongs to a comdat section associated with another section.
       // In both of these cases, we can expect the section to be resolved by
       // the time we finish visiting the remaining symbols in the symbol
       // table. So we postpone the handling of this symbol until that time.
       PendingIndexes.push_back(I);
     }
     I += COFFSym.getNumberOfAuxSymbols();
   }
 
if (Prevailing) {
       C->Sym = cast<DefinedRegular>(Leader);
       cast<DefinedRegular>(Leader)->Data = &C->Repl;
     } else {
       SparseChunks[SectionNumber] = nullptr;
     }
     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);
 }
 
 MachineTypes ObjFile::getMachineType() {
   if (COFFObj)
     return static_cast<MachineTypes>(COFFObj->getMachine());
   return IMAGE_FILE_MACHINE_UNKNOWN;

lld/trunk/test/COFF/associative-comdat-empty.s

+# RUN: yaml2obj < %s > %t.obj
+# RUN: not lld-link /include:symbol /dll /noentry /nodefaultlib %t.obj /out:%t.exe 2>&1 | FileCheck %s
+
+# Tests an associative comdat being associated with an empty section errors.
+# CHECK: lld-link: error: {{.*}}: associative comdat .text$ac1 has invalid reference to section .text$nm
+# CHECK-NOT: lld-link: error:
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            '.text$ac1'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       1
+    SectionData:     '01000000'
+  - Name:            '.text$nm'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       1
+    SectionData:     '01000000'
+symbols:         
+  - Name:            '.text$ac1'
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3099354981
+      Number:          2
+      Selection:       IMAGE_COMDAT_SELECT_ASSOCIATIVE
+  - Name:            '.text$nm'
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3099354981
+      Number:          4
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            assocsym
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...
+
+

lld/trunk/test/COFF/associative-comdat-order.s

+# RUN: yaml2obj < %s > %t.obj
+# RUN: not lld-link /include:symbol /dll /noentry /nodefaultlib %t.obj /out:%t.exe 2>&1 | FileCheck %s
+
+# Tests that an associative comdat being associated with another
+# associated comdat later in the file produces an error.
+# CHECK: lld-link: error: {{.*}}: associative comdat .text$ac1 has invalid reference to section .text$ac2
+# CHECK-NOT: lld-link: error:
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            '.text$ac1'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       1
+    SectionData:     '01000000'
+  - Name:            '.text$ac2'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       1
+    SectionData:     '01000000'
+  - Name:            '.text$nm'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       1
+    SectionData:     '01000000'
+symbols:         
+  - Name:            '.text$ac1'
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3099354981
+      Number:          2
+      Selection:       IMAGE_COMDAT_SELECT_ASSOCIATIVE
+  - Name:            '.text$ac2'
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3099354981
+      Number:          3
+      Selection:       IMAGE_COMDAT_SELECT_ASSOCIATIVE
+  - Name:            '.text$nm'
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3099354981
+      Number:          4
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            symbol
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            assocsym2
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            assocsym
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...
+