[PATCH] D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/ScriptParser.cpp:804
+      std::tie(withFlags, withoutFlags) = readInputSectionFlags();
+      tok = next();
+    }
----------------
grimar wrote:
> The same.
And here.


================
Comment at: lld/ELF/ScriptParser.cpp:863
       readInclude();
+    } else if (tok == "INPUT_SECTION_FLAGS") {
+        std::tie(withFlags, withoutFlags) = readInputSectionFlags();
----------------
peter.smith wrote:
> grimar wrote:
> > We parse "KEEP" in `readInputSectionDescription`, and you handle "INPUT_SECTION_FLAGS" for it right there.
> > Seems you can remove handling of "INPUT_SECTION_FLAGS" from here and move it to `readInputSectionDescription` too
> > (for the no-KEEP case).
> > 
> > This should allow avoid having `withFlags = withoutFlags = 0;` code (and variables itself) around.
> On my first attempt I had the logic in readInputSectionDescription. The problem I found was with the no section description case matched in:
> ```
> 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.
> }
> ```
> As INPUT_SECTION_DESCRIPTION always has parentheses it is matched by
> ```
> else if (peek() == "(") 
> ```
> So if I write: 
> ``` 
> { INPUT_SECTION_DESCRIPTION(some flag list) filespec } 
> ``` 
> it matches the else if (peek() == "("). Unfortunately the above is not easily distinguishable from:
> ```
> { INPUT_SECTION_DESCRIPTION(some flag list) filespec(inputsectiondescription) }
> ```
> I couldn't think of an easy way of distinguishing between these two cases at this level. I'm open to suggestions as I'm not keen on having to clear the state.
I see now, thanks.
There seems to be another problem of having "INPUT_SECTION_FLAGS" here:

I think that scripts like
"{ INPUT_SECTION_DESCRIPTION(list1) INPUT_SECTION_DESCRIPTION(list2) <the_rest_part>}" are accepted?
It is not good.

The solution I have in mind is to refactor the following code
```
      // 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({"*"})});
      cmd->sectionCommands.push_back(isd);
      withFlags = withoutFlags = 0;
```

Into a different function that will also try to handle `INPUT_SECTION_FLAGS`. Sounds like it might work?
I am happy to try to experiment with this (or another approaches), but would like to clarify:

1) Do we need/want to support this case? "We have a file name and no input sections description" case seems
    to be corner case (I am not sure how often it is used). And it is not clear if other linkers support this combination.
    I quess it is probably a reasonable one though.
2) If we want it, then the question is: how much it is important to support this from start? It seems that leaving it for a follow-up
    patch (and changing this place to "first attemp logic" for now) can be a reasonable solution to proceed for now.



================
Comment at: lld/ELF/ScriptParser.cpp:712
+    InputSectionDescription *cmd =
+        readInputSectionRules(tok, withFlags, withoutFlags);
     expect(")");
----------------
I think you can now get rid of `tok = next();`:

```
InputSectionDescription *cmd =
  readInputSectionRules(next(), withFlags, withoutFlags);
```


================
Comment at: lld/test/ELF/input-section-flags-diag3.test:5
+
+## Check that we start with a flag
+
----------------
Seems it belongs to a different test case and needs an update?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72756/new/

https://reviews.llvm.org/D72756