lld-link: Store comdat selection in SectionChunk, reject more invalid associated comdats

[thakis]

Summary

I need the comdat selection for PR40094. To keep the patch for that smaller, I'm adding it here, and as a first application I'm using it to reject associative comdats referring to earlier associative comdats. Depends on D56929; together with that all associative comdats referring to other associative comdats are now rejected.

[thakis]

rebase

[pcc]

lld/COFF/InputFiles.cpp

229 ↗

(On Diff #183014)

For lambdas only used locally within a function we normally use an implicit capture with [&].

258 ↗

(On Diff #183014)

Maybe move this inside readSection and do C->Selection = Def->Selection;?

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

1 ↗

(On Diff #183014)

(Sorry didn't notice this on your other patch)

This isn't an assembly file, so the extension should be .test.

[thakis]

lld/COFF/InputFiles.cpp

258 ↗

(On Diff #183014)

That's a good suggestion, but Def->Selection is an uint8_t while C->Selection is a COMDATType.

I could make readSection() do validation and error out if Def->Selection is something unknown, but in the upcoming patch for better Selection handling I need to do this validation long before I call readSection(), so I'd have to do it in two places then. So I figured setting it here is maybe easier?

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

1 ↗

(On Diff #183014)

Renamed in 352074.

[ruiu]

LGTM

lld/COFF/InputFiles.cpp

229 ↗

(On Diff #183542)

nit: since this function reads associative comdat sections, Assoc in a variable name seems redundant.

[stella.stamenova]

It looks like this change broke the windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1030

For some reason the change isn't logged as one of the ones included in the build, but it was fetched (http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1030/steps/svn-lld/logs/stdio) and built.

[thakis]

In D56931#1372482, @stella.stamenova wrote:

It looks like this change broke the windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1030

For some reason the change isn't logged as one of the ones included in the build, but it was fetched (http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1030/steps/svn-lld/logs/stdio) and built.

Thanks for the note. r352304 will hopefully help.

lld/trunk/COFF/Chunks.h

public:
   const coff_section *Header;
 
   // The file that this chunk was created from.
   ObjFile *File;
 
   // The COMDAT leader symbol if this is a COMDAT chunk.
   DefinedRegular *Sym = nullptr;
 
+  // The COMDAT selection if this is a COMDAT chunk.
+  llvm::COFF::COMDATType Selection;
+
   ArrayRef<coff_relocation> Relocs;
 
   // Used by the garbage collector.
   bool Live;
 
   // When inserting a thunk, we need to adjust a relocation to point to
   // the thunk instead of the actual original target Symbol.
   std::vector<Symbol *> RelocTargets;

lld/trunk/COFF/InputFiles.cpp

void ObjFile::readAssociativeDefinition(
   readAssociativeDefinition(Sym, Def, Def->getNumber(Sym.isBigObj()));
 }
 
 void ObjFile::readAssociativeDefinition(COFFSymbolRef Sym,
                                         const coff_aux_section_definition *Def,
                                         uint32_t ParentIndex) {
   SectionChunk *Parent = SparseChunks[ParentIndex];
 
-  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.
+  auto Diag = [&]() {
     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);
+  };
+
+  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.
+    Diag();
     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 (Parent->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
+      Diag();
+      return;
+    }
+
+    SectionChunk *C = readSection(SectionNumber, Def, "");
+    C->Selection = IMAGE_COMDAT_SELECT_ASSOCIATIVE;
+    SparseChunks[SectionNumber] = C;
     if (SparseChunks[SectionNumber])
       Parent->addAssociative(SparseChunks[SectionNumber]);
   } else {
     SparseChunks[SectionNumber] = nullptr;
   }
 }
 
 void ObjFile::recordPrevailingSymbolForMingw(

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

-# 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:
+# RUN: sed -e s/ASSOC1/2/ -e s/ASSOC2/3/ %s | yaml2obj > %t.obj
+# RUN: not lld-link /include:symbol /dll /noentry /nodefaultlib %t.obj /out:%t.exe 2>&1 | FileCheck --check-prefix=FORWARD %s
+# FORWARD: lld-link: error: {{.*}}: associative comdat .text$ac1 has invalid reference to section .text$ac2
+# FORWARD-NOT: lld-link: error:
+
+# Tests that an associative comdat being associated with another
+# associated comdat earlier in the file produces an error.
+# RUN: sed -e s/ASSOC1/3/ -e s/ASSOC2/1/ %s | yaml2obj > %t.obj
+# RUN: not lld-link /include:symbol /dll /noentry /nodefaultlib %t.obj /out:%t.exe 2>&1 | FileCheck --check-prefix=BACKWARD %s
+# BACKWARD: lld-link: error: {{.*}}: associative comdat .text$ac2 has invalid reference to section .text$ac1
+# BACKWARD-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 ]
- Name:            '.text$ac1'
     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
+      Number:          ASSOC1
       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
+      Number:          ASSOC2
       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: