[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:863
       readInclude();
+    } else if (tok == "INPUT_SECTION_FLAGS") {
+        std::tie(withFlags, withoutFlags) = readInputSectionFlags();
----------------
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.


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

https://reviews.llvm.org/D72756