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

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 3 inline comments as done.
peter.smith added a comment.

I'll remove the support for the corner-case of when there is just a filepattern.  Most likely won't get the time today, if I don't then I'll update on Monday. Thanks for the comments.



================
Comment at: lld/ELF/ScriptParser.cpp:863
       readInclude();
+    } else if (tok == "INPUT_SECTION_FLAGS") {
+        std::tie(withFlags, withoutFlags) = readInputSectionFlags();
----------------
grimar wrote:
> grimar wrote:
> > 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.
> > 
> >> The solution I have in mind is to refactor the following code
> 
> Ah, and this will not work because of the reason you've mentioned. Sorry.
> I'd really leave this corner-case for a follow-up patch I think.
I was just following the ld.bfd grammar, so I'm happy to remove it and leave it for a follow up patch if it were necessary. 


================
Comment at: lld/ELF/ScriptParser.cpp:712
+    InputSectionDescription *cmd =
+        readInputSectionRules(tok, withFlags, withoutFlags);
     expect(")");
----------------
grimar wrote:
> I think you can now get rid of `tok = next();`:
> 
> ```
> InputSectionDescription *cmd =
>   readInputSectionRules(next(), withFlags, withoutFlags);
> ```
I don't think I can, that line is the same as line 699 in the diff on the left hand side.

```
    StringRef filePattern = next();
    if (consume("INPUT_SECTION_FLAGS"))
```
I think I can rename tok to filePattern though.


================
Comment at: lld/test/ELF/input-section-flags-diag3.test:5
+
+## Check that we start with a flag
+
----------------
grimar wrote:
> Seems it belongs to a different test case and needs an update?
Thanks I'll update the comment.


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

https://reviews.llvm.org/D72756