Page MenuHomePhabricator

[lld/ELF] PR44498: Support input filename in double quote
ClosedPublic

Authored by thopre on Jan 10 2020, 9:09 AM.

Details

Summary

Linker scripts allow filenames to be put in double quotes to prevent
characters in filenames that are part of the linker script syntax from
having their special meaning. Case in point the * wildcard character.

Availability of double quoting filenames also allows to fix a failure in
ELF/linkerscript/filename-spec.s when the path contain a @ which the
lexer consider as a special characters and thus break up a filename
containing it. This may happens under Jenkins which createspath such as
pipeline@2.

To avoid the need for escaping GlobPattern metacharacters in filename
in double quotes, GlobPattern::create is augmented with a new parameter
to request literal matching instead of relying on the presence of a
wildcard character in the pattern.

Diff Detail

Event Timeline

thopre created this revision.Jan 10 2020, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 9:09 AM
thopre retitled this revision from [lld/ELF] Support input filename in double quote to [lld/ELF] PR44498: Support input filename in double quote.Jan 10 2020, 9:10 AM

I think we should add logic to StringMatcher instead of GlobPattern.

lld/test/ELF/linkerscript/filename-spec.s
33

Un-quote this test so that we won't lose coverage for unquoted strings.

allows to fix a failure in ELF/linkerscript/filename-spec.s when the path contain a @ which the lexer consider as a special characters and thus break up a filename containing it.

Which one?

evgeny777 added inline comments.Jan 13 2020, 1:26 AM
llvm/include/llvm/Support/GlobPattern.h
31 ↗(On Diff #237357)

This looks counter intuitive. Typically I'd use GlobPattern for wildcard matching, not for exact comparison.

thopre updated this revision to Diff 238037.Jan 14 2020, 10:45 AM
  • Make exact match at StringMatcher level
  • Add possibility to add new pattern to a StringMatcher
thopre edited the summary of this revision. (Show Details)Jan 14 2020, 10:47 AM
thopre marked 2 inline comments as done.Jan 14 2020, 11:01 AM
allows to fix a failure in ELF/linkerscript/filename-spec.s when the path contain a @ which the lexer consider as a special characters and thus break up a filename containing it.

Which one?

See updated description. Jenkins sometimes create workspace with an @2 in them so if the repo is cloned into that path test filename will have an @ which will confuse the lexer on that line. It'll break the filename just before the @ and will create a matcher for that rather than the full path.

lld/test/ELF/linkerscript/filename-spec.s
33

The problem is that this patch was created precisely to deal with this line failing when the path to the test contains a special character, which happens under jenkins (see updated description).

I think that adding extra flag to GlobPattern::create was better idea. Just set it to false by default, so that GlobPattern will still act as glob matcher for all other users:

static static Expected<GlobPattern> create(StringRef Pat, bool IgnoreMetaChars = false);

Than add extra constructor to StringMatcher to which you'll pass GlobPattern and use it from InputSectionDescription constructor.

lld/ELF/ScriptParser.cpp
643

Add empty() method to StringMatcher and use !SectionMatcher.empty() instead of this flag

868

What's the purpose of this change?

thopre marked 4 inline comments as done.Jan 16 2020, 3:11 PM

I think that adding extra flag to GlobPattern::create was better idea. Just set it to false by default, so that GlobPattern will still act as glob matcher for all other users:

static static Expected<GlobPattern> create(StringRef Pat, bool IgnoreMetaChars = false);

Than add extra constructor to StringMatcher to which you'll pass GlobPattern and use it from InputSectionDescription constructor.

Actually I agree with Fangrui. Having GlobPattern do any match other than globbing would be counter-intuitive given the name. A user who doesn't want globbing just use an exact match. I'm not opposed to create a new wrapper class around GlobPattern that would allow exact match with a name that reflect that matching duality. Maybe simply PatternMatcher?

lld/ELF/ScriptParser.cpp
868

I've changed StringMatcher to only take a single pattern to avoid changing the array of pattern for an array of pair (pattern + exact match boolean) which would make the interface more cumbersome to use *and* implement. All callers passing an array were constructing it in a loop with push_back which I've changed for a call to a new addPattern.

thopre updated this revision to Diff 238646.Jan 16 2020, 3:16 PM
thopre marked an inline comment as done.

Introduce StringMatcher.empty()

MaskRay accepted this revision.Jan 17 2020, 5:56 PM

LG. It may be worth waiting for other opinions.

This revision is now accepted and ready to land.Jan 17 2020, 5:56 PM
This revision was automatically updated to reflect the committed changes.