Address review feedback

- Add an explicit test that CLASS(a b) doesn't parse as a reference.
- Add SORT_BY_ALIGNMENT in class def to demonstrate more complex
  matching.
- Rename InputSectionDescription className to classRef to make it
  clearer that the class is being referenced, not defined.
- Define the interaction between --enable-non-contiguous-regions and
  CLASS and add tests.
- Add alreadyAssignedToOutCmd lambda to centralize comments.

Author: mysterymath

lld/ELF/LinkerScript.cpp

@@ -497,13 +497,20 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
   SmallVector<InputSectionBase *, 0> ret;
   DenseSet<InputSectionBase *> spills;
 
+  // Returns whether an input section was already assigned to an earlier input
+  // section description in this output section or section class.
+  const auto alreadyAssignedToOutCmd =
+      [&outCmd](InputSectionBase *sec) { return sec->parent == &outCmd; };
+
+  // Returns whether an input section's flags match the input section
+  // description's specifiers.
   const auto flagsMatch = [cmd](InputSectionBase *sec) {
     return (sec->flags & cmd->withFlags) == cmd->withFlags &&
            (sec->flags & cmd->withoutFlags) == 0;
   };
 
   // Collects all sections that satisfy constraints of Cmd.
-  if (cmd->className.empty()) {
+  if (cmd->classRef.empty()) {
     DenseSet<size_t> seen;
     size_t sizeAfterPrevSort = 0;
     SmallVector<size_t, 0> indexes;
@@ -540,30 +547,24 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
           continue;
 
         if (!cmd->matchesFile(sec->file) || pat.excludesFile(sec->file) ||
-            !flagsMatch(sec))
+            alreadyAssignedToOutCmd(sec) || !flagsMatch(sec))
           continue;
 
         if (sec->parent) {
           // Skip if not allowing multiple matches.
           if (!config->enableNonContiguousRegions)
             continue;
 
-          // Disallow spilling out of or into section classes; that's already a
-          // mechanism for spilling.
-          if (isa<SectionClass>(sec->parent) || isa<SectionClass>(outCmd))
-            continue;
-
           // Disallow spilling into /DISCARD/; special handling would be needed
           // for this in address assignment, and the semantics are nebulous.
           if (outCmd.name == "/DISCARD/")
             continue;
 
-          // Skip if the section was already matched by a different input
-          // section description within this output section or class.
-          if (sec->parent == &outCmd)
-            continue;
-
-          spills.insert(sec);
+          // Class definitions cannot contain spills, nor can a class definition
+          // generate a spill in a subsequent match. Those behaviors belong to
+          // class references and additional matches.
+          if (!isa<SectionClass>(outCmd) && !isa<SectionClass>(sec->parent))
+            spills.insert(sec);
         }
 
         ret.push_back(sec);
@@ -594,20 +595,20 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
     // input order).
     sortByPositionThenCommandLine(sizeAfterPrevSort, ret.size());
   } else {
-    SectionClassDesc *scd = script->sectionClasses.lookup(cmd->className);
+    SectionClassDesc *scd = script->sectionClasses.lookup(cmd->classRef);
     if (!scd) {
-      error("undefined section class '" + cmd->className + "'");
+      error("undefined section class '" + cmd->classRef + "'");
       return ret;
     }
     if (!scd->sc.assigned) {
-      error("section class '" + cmd->className + "' referenced by '" +
+      error("section class '" + cmd->classRef + "' referenced by '" +
             outCmd.name + "' before class definition");
       return ret;
     }
 
     for (InputSectionDescription *isd : scd->sc.commands) {
       for (InputSectionBase *sec : isd->sectionBases) {
-        if (sec->parent == &outCmd || !flagsMatch(sec))
+        if (alreadyAssignedToOutCmd(sec) || !flagsMatch(sec))
           continue;
         bool isSpill = sec->parent && isa<OutputSection>(sec->parent);
         if (!sec->parent || (isSpill && outCmd.name == "/DISCARD/"))
@@ -756,8 +757,12 @@ void LinkerScript::processSectionCommands() {
       for (InputSectionDescription *isd : sc->sc.commands) {
         isd->sectionBases =
             computeInputSections(isd, ctx.inputSections, sc->sc);
-        for (InputSectionBase *s : isd->sectionBases)
-          s->parent = &sc->sc;
+        for (InputSectionBase *s : isd->sectionBases) {
+          // Section classes with --enable-non-contiguous-regions may contain
+          // parented classes; spills for these are generated on reference.
+          if (!s->parent)
+            s->parent = &sc->sc;
+        }
       }
       sc->sc.assigned = true;
     }

lld/ELF/LinkerScript.h

@@ -201,11 +201,11 @@ class InputSectionDescription : public SectionCommand {
 
 public:
   InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
-                          uint64_t withoutFlags = 0, StringRef className = {})
+                          uint64_t withoutFlags = 0, StringRef classRef = {})
       : SectionCommand(InputSectionKind), filePat(filePattern),
-        withFlags(withFlags), withoutFlags(withoutFlags), className(className) {
-    assert((filePattern.empty() || className.empty()) &&
-           "file pattern and class name are mutually exclusive");
+        withFlags(withFlags), withoutFlags(withoutFlags), classRef(classRef) {
+    assert((filePattern.empty() || classRef.empty()) &&
+           "file pattern and class reference are mutually exclusive");
   }
 
   static bool classof(const SectionCommand *c) {
@@ -237,7 +237,7 @@ class InputSectionDescription : public SectionCommand {
 
   // If present, input section matching uses class membership instead of file
   // and section patterns (mutually exclusive).
-  StringRef className;
+  StringRef classRef;
 };
 
 // Represents BYTE(), SHORT(), LONG(), or QUAD().

lld/ELF/ScriptParser.cpp

@@ -620,9 +620,9 @@ SectionClassDesc *ScriptParser::readSectionClassDescription() {
       setError("expected filename pattern");
     else if (peek() == "(") {
       InputSectionDescription *isd = readInputSectionDescription(tok);
-      if (!isd->className.empty())
+      if (!isd->classRef.empty())
         setError("section class '" + name + "' references class '" +
-                 isd->className + "'");
+                 isd->classRef + "'");
       desc->sc.commands.push_back(isd);
     }
   }

lld/test/ELF/linkerscript/section-class.test

@@ -12,7 +12,7 @@
 # RUN: llvm-readobj -x .rodata -x .rodata.d matching | FileCheck %s --check-prefix=MATCHING
 
 # MATCHING:      .rodata
-# MATCHING-NEXT: 020301
+# MATCHING-NEXT: 020301cc 0605
 # MATCHING:      .rodata.d
 # MATCHING-NEXT: 04
 
@@ -34,6 +34,14 @@
 # MISSING-FILENAME-PATTERN: error: missing-filename-pattern.ld:2: expected filename pattern
 
 
+## An error is reported when more than one class is mentioned in a reference.
+
+# RUN: not ld.lld -T multiple-class-names.ld matching.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MULTIPLE-CLASS-NAMES --implicit-check-not=error:
+
+# MULTIPLE-CLASS-NAMES: error: multiple-class-names.ld:4: ) expected, but got b
+
+
 ## An error is reported when the content of section classes is demanded before
 ## its definition is processed.
 
@@ -113,6 +121,19 @@
 # SPILL-EARLIER-NEXT: .last_chance  PROGBITS 0000000000000002 001002 000001
 
 
+## Class definitions do not preclude additional matches when used with
+## --enable-non-contiguous-regions, and additional matches in class
+## definitions become spills at class references.
+
+# RUN: ld.lld -T enable-non-contiguous-regions.ld spill.o -o enable-non-contiguous-regions --enable-non-contiguous-regions
+# RUN: llvm-readelf -S enable-non-contiguous-regions | FileCheck %s --check-prefix=ENABLE-NON-CONTIGUOUS-REGIONS
+
+# ENABLE-NON-CONTIGUOUS-REGIONS:      Name          Type     Address          Off    Size
+# ENABLE-NON-CONTIGUOUS-REGIONS:      .first_chance     PROGBITS 0000000000000000 000190 000000
+# ENABLE-NON-CONTIGUOUS-REGIONS-NEXT: .last_chance      PROGBITS 0000000000000001 001001 000002
+# ENABLE-NON-CONTIGUOUS-REGIONS-NEXT: .one_byte_section PROGBITS 0000000000000003 001003 000001
+
+
 ## SHF_MERGEd sections are spilled according to the class refs of the first
 ## merged input section (the one giving the resulting section its name).
 
@@ -179,14 +200,23 @@
 .section .rodata.d,"a",@progbits
 .byte 4
 
+.section .rodata.e,"a",@progbits
+.byte 5
+
+.section .rodata.f,"a",@progbits
+.balign 2
+.byte 6
+
 #--- matching.ld
 SECTIONS {
   CLASS(a) { *(.rodata.a) }
   CLASS(cd) { *(.rodata.c) *(.rodata.d) }
+  CLASS(ef) { *(SORT_BY_ALIGNMENT(.rodata.e .rodata.f)) }
   .rodata : {
     *(.rodata.*)
     INPUT_SECTION_FLAGS(SHF_EXECINSTR) CLASS(cd)
     CLASS(a)
+    CLASS(ef)
   }
   OVERLAY : { .rodata.d { INPUT_SECTION_FLAGS(!SHF_EXECINSTR) CLASS(cd) } }
 }
@@ -202,6 +232,13 @@ SECTIONS {
   CLASS(a) { (.rodata.a) }
 }
 
+#--- multiple-class-names.ld
+SECTIONS {
+  CLASS(a) { *(.rodata.a) }
+  CLASS(b) { *(.rodata.b) }
+  .rodata : { CLASS(a b) }
+}
+
 #--- referenced-before-defined.ld
 SECTIONS {
   .rodata : { CLASS(a) }
@@ -290,6 +327,25 @@ SECTIONS {
   .last_chance : { CLASS(c) } >b
 }
 
+#--- enable-non-contiguous-regions.ld
+MEMORY {
+  a : ORIGIN = 0, LENGTH = 1
+  b : ORIGIN = 1, LENGTH = 2
+  c : ORIGIN = 3, LENGTH = 1
+}
+
+SECTIONS {
+  .first_chance : { *(.two_byte_section) } >a
+  /* An additional match in a class defers a spill. */
+  CLASS(two) { *(.two_byte_section) }
+  /* A class references actualizes deferred spills. */
+  .last_chance : { CLASS(two) } >b
+
+  /* Section classes do not preclude other matches. */
+  CLASS(one) { *(.one_byte_section) }
+  .one_byte_section : { *(.one_byte_section) } >c
+}
+
 #--- merge.s
 .section .a,"aM",@progbits,1
 .byte 0x12, 0x34