[LLD][ELF] - Linkerscript: support the case when INPUT_SECTION_FLAGS is used without section patterns.

[grimar]

Summary

This is a continuation of D72756. It uses one of test cases made by Peter Smith which was
not included to the final version of the patch (we've agreed to place a FIXME that time).

This patch suggests a change to support this missing part.
(I am not sure how much it is useful feature, but I think we do not want to have this FIXME
and IMO this change makes the code a bit cleaner).

[grimar]

• Move the new test case to a proper position in the file.

[psmith]

I've made a comment about potentially allowing nested KEEP, although I'm not 100% sure about it. Other than that this looks good.

lld/ELF/ScriptParser.cpp

702

If I've read this right, this would allow something like:

KEEP ( INPUT_SECTION_FLAGS KEEP ( ... ) )

Although not pretty, perhaps a bool parameter to readInputSectionDescription to say if we are within a KEEP so we can error if we have another. I think the ldgram.y has a separate input_section_no_keep that it uses for this purpose.

[MaskRay]

This syntax probably has no user now. The question is whether the syntax can have a reasonable use case in the future... The raw wildcard syntax is questionable because it can potentially cause parsing ambiguity. Having ) as a terminal symbol is nice.

I also left a comment on https://bugs.llvm.org/show_bug.cgi?id=39885

[grimar]

In D76765#1941605, @MaskRay wrote:

The raw wildcard syntax is questionable because it can potentially cause parsing ambiguity.
...
I also left a comment on https://bugs.llvm.org/show_bug.cgi?id=39885

What about if we drop the following piece we have then?

} else {
  // We have a file name and no input sections description. It is not a
  // commonly used syntax, but still acceptable. In that case, all sections
  // from the file will be included.
  // FIXME: GNU ld permits INPUT_SECTION_FLAGS to be used here. We do not
  // handle this case here as it will already have been matched by the
  // case above.
  auto *isd = make<InputSectionDescription>(tok);
  isd->sectionPatterns.push_back({{}, StringMatcher("*")});
  cmd->sectionCommands.push_back(isd);
}

[psmith]

In D76765#1943051, @grimar wrote:

In D76765#1941605, @MaskRay wrote:

The raw wildcard syntax is questionable because it can potentially cause parsing ambiguity.
...
I also left a comment on https://bugs.llvm.org/show_bug.cgi?id=39885

What about if we drop the following piece we have then?

} else {
  // We have a file name and no input sections description. It is not a
  // commonly used syntax, but still acceptable. In that case, all sections
  // from the file will be included.
  // FIXME: GNU ld permits INPUT_SECTION_FLAGS to be used here. We do not
  // handle this case here as it will already have been matched by the
  // case above.
  auto *isd = make<InputSectionDescription>(tok);
  isd->sectionPatterns.push_back({{}, StringMatcher("*")});
  cmd->sectionCommands.push_back(isd);
}

Given that someone can always write the pattern with ("*") which will also work on binutils then I'm happy to say we don't support it. If we do drop support we should consider a "not supported" error message, and to document it as a known limitation.

[grimar]

Abandining since seems we want to drop the support of syntax when we have a file name and no input sections description and that is not what this patch does.

lld/ELF/ScriptParser.cpp

ScriptParser::readInputSectionRules(StringRef filePattern, uint64_t withFlags,
   }
   return cmd;
 }
 
 InputSectionDescription *
 ScriptParser::readInputSectionDescription(StringRef tok) {
   // Input section wildcard can be surrounded by KEEP.
   // https://sourceware.org/binutils/docs/ld/Input-Section-Keep.html#Input-Section-Keep
-  uint64_t withFlags = 0;
-  uint64_t withoutFlags = 0;
   if (tok == "KEEP") {
     expect("(");
-    if (consume("INPUT_SECTION_FLAGS"))
-      std::tie(withFlags, withoutFlags) = readInputSectionFlags();
     InputSectionDescription *cmd =
-        readInputSectionRules(next(), withFlags, withoutFlags);
+        peek() == "INPUT_SECTION_FLAGS"
+            ? readInputSectionDescription(next())
+            : readInputSectionRules(next(), /*withFlags=*/0,
+                                    /*withoutFlags*/ 0);
     expect(")");
     script->keptSections.push_back(cmd);
     return cmd;
   }
+
+  uint64_t withFlags = 0;
+  uint64_t withoutFlags = 0;
   if (tok == "INPUT_SECTION_FLAGS") {
     std::tie(withFlags, withoutFlags) = readInputSectionFlags();
     tok = next();
   }
+
+  if (peek() != "(") {
+    // We have a file name and no input sections description. It is not a
+    // commonly used syntax, but still acceptable. In that case, all sections
+    // from the file will be included.
+    auto *isd = make<InputSectionDescription>(tok, withFlags, withoutFlags);
+    isd->sectionPatterns.push_back({{}, StringMatcher("*")});
+    return isd;
+  }
+
   return readInputSectionRules(tok, withFlags, withoutFlags);
 }
 
 void ScriptParser::readSort() {
   expect("(");
   expect("CONSTRUCTORS");
   expect(")");
 }
if (tok == ";") {
       // https://sourceware.org/binutils/docs/ld/Output-Section-Data.html
       if (peek() != "(")
         setError("( expected, but got " + peek());
       cmd->filler = readFill();
     } else if (tok == "SORT") {
       readSort();
     } else if (tok == "INCLUDE") {
       readInclude();
-    } else if (peek() == "(") {
-      cmd->sectionCommands.push_back(readInputSectionDescription(tok));
     } else {
-      // We have a file name and no input sections description. It is not a
-      // commonly used syntax, but still acceptable. In that case, all sections
-      // from the file will be included.
-      // FIXME: GNU ld permits INPUT_SECTION_FLAGS to be used here. We do not
-      // handle this case here as it will already have been matched by the
-      // case above.
-      auto *isd = make<InputSectionDescription>(tok);
-      isd->sectionPatterns.push_back({{}, StringMatcher("*")});
-      cmd->sectionCommands.push_back(isd);
+      cmd->sectionCommands.push_back(readInputSectionDescription(tok));
     }
   }
 
   if (consume(">"))
     cmd->memoryRegionName = std::string(next());
 
   if (consume("AT")) {
     expect(">");

lld/test/ELF/input-section-flags.s

 # MULTIPLE:  Section: .outsec1
 # MULTIPLE:  Name: s3
 # MULTIPLE:  Section: .sec.aw
 # MULTIPLE:  Name: s4
 # MULTIPLE:  Section: .sec.aM
 # MULTIPLE:  Name: s5
 # MULTIPLE:  Section: .sec.aMS
 
+## Same test but using just a filespec.
+# RUN: echo "SECTIONS { \
+# RUN: .outsec1 : { INPUT_SECTION_FLAGS(SHF_ALLOC & !SHF_EXECINSTR & \
+# RUN:                                  !SHF_WRITE & !SHF_MERGE) *.o }\
+# RUN: .outsec2 : { INPUT_SECTION_FLAGS(SHF_ALLOC & SHF_EXECINSTR & !SHF_WRITE\
+# RUN:                                  & !SHF_MERGE) *.o } \
+# RUN: .outsec3 : { INPUT_SECTION_FLAGS(SHF_WRITE) *.o } \
+# RUN: .outsec4 : { INPUT_SECTION_FLAGS(SHF_MERGE & !SHF_STRINGS) * } \
+# RUN: .outsec5 : { INPUT_SECTION_FLAGS(SHF_STRINGS) * } \
+# RUN: } " > %t5.script
+
+# RUN: ld.lld -o %t5 --script %t5.script %t.o
+# RUN: llvm-readobj --symbols %t5 | FileCheck %s
+
  .text
  .global _start
 _start:
  nop
 
  .section .sec.a, "a", @progbits
  .globl s1
 s1: