Page MenuHomePhabricator

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

Authored by grimar on Mar 25 2020, 5:00 AM.

Details

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).

Diff Detail

Event Timeline

grimar created this revision.Mar 25 2020, 5:00 AM
grimar updated this revision to Diff 252549.Mar 25 2020, 5:18 AM
  • Move the new test case to a proper position in the file.

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 added a comment.EditedMar 25 2020, 9:59 AM

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

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);
}
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 abandoned this revision.EditedApr 10 2020, 1:05 AM

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.