Address review comments

- Remove trailing hashes from test
- Joined test class references together to demonstrate spaces not required
- Migrate to llvm-objdump -s for content tests; add end of line checks
- Add test for missing open paren not being interpreted as a filename
- Add test for undefined section class
- Use abbreviation "osd" for OutputDesc variables
- Add comment for section class command enum
- Move classRef member variable earlier in InputSectionDescription
- Use DenseMap from CachedHashStringRef over StringMap
- Clarify undefined.lds test comment.
- Clarify comment for section class members with different parent
- Replace comment on SectionClass struct
- Use unquote for section names; add test

Author: mysterymath

lld/ELF/LinkerScript.cpp

@@ -592,7 +592,8 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
     // input order).
     sortByPositionThenCommandLine(sizeAfterPrevSort, ret.size());
   } else {
-    SectionClassDesc *scd = script->sectionClasses.lookup(cmd->classRef);
+    SectionClassDesc *scd =
+        script->sectionClasses.lookup(CachedHashStringRef(cmd->classRef));
     if (!scd) {
       errorOrWarn("undefined section class '" + cmd->classRef + "'");
       return ret;
@@ -758,8 +759,9 @@ void LinkerScript::processSectionCommands() {
         isd->sectionBases =
             computeInputSections(isd, ctx.inputSections, 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.
+          // A section class containing a section with different parent isn't
+          // necessarily an error due to --enable-non-contiguous-regions. Such
+          // sections all become potential spills when the class is referenced.
           if (!s->parent)
             s->parent = &sc->sc;
         }
@@ -833,8 +835,8 @@ void LinkerScript::processSymbolAssignments() {
   for (SectionCommand *cmd : sectionCommands) {
     if (auto *assign = dyn_cast<SymbolAssignment>(cmd))
       addSymbol(assign);
-    else if (auto *od = dyn_cast<OutputDesc>(cmd))
-      for (SectionCommand *subCmd : od->osec.commands)
+    else if (auto *osd = dyn_cast<OutputDesc>(cmd))
+      for (SectionCommand *subCmd : osd->osec.commands)
         if (auto *assign = dyn_cast<SymbolAssignment>(subCmd))
           addSymbol(assign);
   }
@@ -1531,10 +1533,10 @@ bool LinkerScript::spillSections() {
 
   bool spilled = false;
   for (SectionCommand *cmd : reverse(sectionCommands)) {
-    auto *od = dyn_cast<OutputDesc>(cmd);
-    if (!od)
+    auto *osd = dyn_cast<OutputDesc>(cmd);
+    if (!osd)
       continue;
-    OutputSection *osec = &od->osec;
+    OutputSection *osec = &osd->osec;
     if (!osec->memRegion)
       continue;
 

lld/ELF/LinkerScript.h

@@ -81,7 +81,7 @@ enum SectionsCommandKind {
   OutputSectionKind,
   InputSectionKind,
   ByteKind, // BYTE(expr), SHORT(expr), LONG(expr) or QUAD(expr)
-  ClassKind,
+  ClassKind, // CLASS(class_name)
 };
 
 struct SectionCommand {
@@ -203,7 +203,7 @@ class InputSectionDescription : public SectionCommand {
   InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
                           uint64_t withoutFlags = 0, StringRef classRef = {})
       : SectionCommand(InputSectionKind), filePat(filePattern),
-        withFlags(withFlags), withoutFlags(withoutFlags), classRef(classRef) {
+        classRef(classRef), withFlags(withFlags), withoutFlags(withoutFlags) {
     assert((filePattern.empty() || classRef.empty()) &&
            "file pattern and class reference are mutually exclusive");
   }
@@ -218,6 +218,10 @@ class InputSectionDescription : public SectionCommand {
   // will be associated with this InputSectionDescription.
   SmallVector<SectionPattern, 0> sectionPatterns;
 
+  // If present, input section matching uses class membership instead of file
+  // and section patterns (mutually exclusive).
+  StringRef classRef;
+
   // Includes InputSections and MergeInputSections. Used temporarily during
   // assignment of input sections to output sections.
   SmallVector<InputSectionBase *, 0> sectionBases;
@@ -234,10 +238,6 @@ class InputSectionDescription : public SectionCommand {
   // SectionPatterns can be filtered with the INPUT_SECTION_FLAGS command.
   uint64_t withFlags;
   uint64_t withoutFlags;
-
-  // If present, input section matching uses class membership instead of file
-  // and section patterns (mutually exclusive).
-  StringRef classRef;
 };
 
 // Represents BYTE(), SHORT(), LONG(), or QUAD().
@@ -442,7 +442,7 @@ class LinkerScript final {
   // Named lists of input sections that can be collectively referenced in output
   // section descriptions. Multiple references allow for sections to spill from
   // one output section to another.
-  llvm::StringMap<SectionClassDesc *> sectionClasses;
+  llvm::DenseMap<llvm::CachedHashStringRef, SectionClassDesc *> sectionClasses;
 };
 
 struct ScriptWrapper {

lld/ELF/OutputSections.h

@@ -143,9 +143,9 @@ struct OutputDesc final : SectionCommand {
   }
 };
 
-// A list of input sections that can be referenced in output descriptions.
-// Multiple references allow sections to spill from one output section to the
-// next.
+// This represents a CLASS(class_name) { ... } that can be referenced by output
+// section descriptions. If referenced more than once, the sections can be
+// spilled to the next reference like --enable-non-contiguous-regions.
 struct SectionClass final : public SectionBase {
   SmallVector<InputSectionDescription *, 0> commands;
   bool assigned = false;

lld/ELF/ScriptParser.cpp

@@ -610,7 +610,7 @@ SmallVector<SectionCommand *, 0> ScriptParser::readOverlay() {
 SectionClassDesc *ScriptParser::readSectionClassDescription() {
   StringRef name = readSectionClassName();
   SectionClassDesc *desc = make<SectionClassDesc>(name);
-  if (!script->sectionClasses.insert({name, desc}).second)
+  if (!script->sectionClasses.insert({CachedHashStringRef(name), desc}).second)
     setError("section class '" + name + "' already defined");
   expect("{");
   while (!errorCount() && !consume("}")) {
@@ -630,7 +630,7 @@ SectionClassDesc *ScriptParser::readSectionClassDescription() {
 
 StringRef ScriptParser::readSectionClassName() {
   expect("(");
-  StringRef name = next();
+  StringRef name = unquote(next());
   expect(")");
   return name;
 }

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

@@ -32,38 +32,37 @@
 
 #--- matching.lds
 ## CLASS definitions match sections in linker script order. The sections may be
-## placed in a different order. Classes may derive from one another. Class # #
+## placed in a different order. Classes may derive from one another. Class
 ## references can be restricted by INPUT_SECTION_FLAGS. Classes can be referenced
 ## in /DISCARD/ and INSERT.
 SECTIONS {
   CLASS(a) { *(.rodata.a) }
   CLASS(cd) { *(.rodata.c) *(.rodata.d) }
   CLASS(ef) { *(SORT_BY_ALIGNMENT(.rodata.e .rodata.f)) }
   CLASS(g) { *(.rodata.g) }
-  CLASS(h) { *(.rodata.h) }
+  CLASS("h)") { *(.rodata.h) }
   .rodata : {
     *(.rodata.*)
     INPUT_SECTION_FLAGS(SHF_EXECINSTR) CLASS( cd)
-    CLASS(a)
-    CLASS(ef )
+    CLASS(a)CLASS(ef )
   }
   OVERLAY : { .rodata.d { INPUT_SECTION_FLAGS(!SHF_EXECINSTR) CLASS(cd) } }
   /DISCARD/ : { CLASS(g) }
 }
 
 SECTIONS {
-  .rodata.h : { CLASS(h) }
+  .rodata.h : { CLASS("h)") }
 } INSERT AFTER .rodata;
 
 # RUN: ld.lld -T matching.lds matching.o -o matching
-# RUN: llvm-readobj -x .rodata -x .rodata.d -x .rodata.h matching |\
+# RUN: llvm-objdump -s matching |\
 # RUN:   FileCheck %s --check-prefix=MATCHING
 # MATCHING:      .rodata
-# MATCHING-NEXT: 020301cc 0605
+# MATCHING-NEXT: 020301cc 0605 ......{{$}}
 # MATCHING:      .rodata.h
-# MATCHING-NEXT: 08
+# MATCHING-NEXT: 08 .{{$}}
 # MATCHING:      .rodata.d
-# MATCHING-NEXT: 04
+# MATCHING-NEXT: 04 .{{$}}
 
 #--- already-defined.lds
 ## A section class has more than one description.
@@ -79,16 +78,23 @@ SECTIONS {
 
 # ALREADY-DEFINED: error: already-defined.lds:4: section class 'a' already defined
 
-#--- missing-filename-pattern.lds
+#--- missing-filename-pattern-1.lds
 ## A filename pattern is missing in a section class description.
 SECTIONS {
   CLASS(a) { (.rodata.a) }
 }
+#--- missing-filename-pattern-2.lds
+## A filename pattern is missing in a section class description.
+SECTIONS {
+  CLASS(a) { .rodata.a) }
+}
 
-# RUN: not ld.lld -T missing-filename-pattern.lds matching.o 2>&1 | \
+# RUN: not ld.lld -T missing-filename-pattern-1.lds matching.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING-FILENAME-PATTERN --implicit-check-not=error:
+# RUN: not ld.lld -T missing-filename-pattern-2.lds matching.o 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=MISSING-FILENAME-PATTERN --implicit-check-not=error:
 
-# MISSING-FILENAME-PATTERN: error: missing-filename-pattern.lds:3: expected filename pattern
+# MISSING-FILENAME-PATTERN: error: missing-filename-pattern-{{[1-2]}}.lds:3: expected filename pattern
 
 #--- multiple-class-names.lds
 ## More than one class is mentioned in a reference.
@@ -103,6 +109,17 @@ SECTIONS {
 
 # MULTIPLE-CLASS-NAMES: error: multiple-class-names.lds:5: ) expected, but got b
 
+#--- undefined.lds
+## A section class is referenced but never defined
+SECTIONS {
+  .rodata : { CLASS(a) }
+}
+
+# RUN: not ld.lld -T undefined.lds matching.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=UNDEFINED --implicit-check-not=error:
+
+# UNDEFINED: error: undefined section class 'a'
+
 #--- referenced-before-defined.lds
 ## The content of section classes is demanded before its definition is processed.
 SECTIONS {
@@ -119,8 +136,7 @@ SECTIONS {
 # REFERENCED-BEFORE-DEFINED-WARN: warning: section class 'a' referenced by '.rodata' before class definition
 
 #--- unreferenced.lds
-## An input section is bound to a section class but is not referenced by at
-## least one output section.
+## An input section is bound to a section class but is not referenced.
 SECTIONS {
   CLASS(a) { *(.rodata.*) }
 }
@@ -362,9 +378,9 @@ SECTIONS {
 }
 
 # RUN: ld.lld -T link-order.lds link-order.o -o link-order
-# RUN: llvm-readobj -x .order link-order | FileCheck %s --check-prefix=LINK-ORDER
+# RUN: llvm-objdump -s link-order | FileCheck %s --check-prefix=LINK-ORDER
 
-# LINK-ORDER: 020301
+# LINK-ORDER: 020301 ...{{$}}
 
 #--- from-insert.lds
 ## A section might spill from INSERT.