Skip to content

[FileCheck] improve prefix validation #92248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[FileCheck] improve prefix validation #92248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6e9d93
fix prefix regex validation
klensy May 15, 2024
512b5a7
filecheck: forbid filecheck prefix to ends with directive name
klensy May 15, 2024
16ee61b
filecheck: fix self tests for "forbid prefixes end with directive name"
klensy May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions llvm/lib/FileCheck/FileCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2468,24 +2468,36 @@ FileCheckString::CheckDag(const SourceMgr &SM, StringRef Buffer,

static bool ValidatePrefixes(StringRef Kind, StringSet<> &UniquePrefixes,
ArrayRef<StringRef> SuppliedPrefixes) {
static const char *Suffixes[] = {"-NEXT", "-SAME", "-EMPTY", "-NOT",
"-COUNT", "-DAG", "-LABEL"};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a place that checks for some (but not all) cases of multiple suffixes. That place should use the same list of suffixes that this function does, that is, there should be one list that is shared between them. Ideally that other place would also be enhanced to check for all combinations, but that can wait for another patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which place exactly? I don't very knowledgeable about codebase.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search for "DAG-NOT" and you will see the place I mean. Looks like it specifically checks for NOT combined with anything else, but not other combinations. FileCheck does not have defined semantics for any combination of suffixes.

for (StringRef Prefix : SuppliedPrefixes) {
if (Prefix.empty()) {
errs() << "error: supplied " << Kind << " prefix must not be the empty "
<< "string\n";
return false;
}
static const Regex Validator("^[a-zA-Z0-9_-]*$");
// TODO: restrict prefixes to start with only letter eventually
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete this TODO. We DO NOT want to restrict this: there is no benefit to doing so.

Please move adjusting the error message to match the actual behaviour into a separate PR, as it is not related to "improving prefix validation".

static const Regex Validator("^[a-zA-Z0-9][a-zA-Z0-9_-]*$");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to change the behaviour? What's wrong with a leading underscore or dash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected for prefixes to be kind-of-ident (which can't start from digits and dashes, but digits actually used, bc no check for that was added, oh, well), and be similar to substitution blocks (which should be "[a-zA-Z_][0-9a-zA-Z_]*", as per doc).

Less restrictions makes it harder to catch possible errors in directives; currently possible typos simply ignored.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most languages, identifiers can start with a leading underscore (this is sometimes reserved by the system, but it's still a valid identifier). FileCheck's behaviour essentially treats dashes like underscores (hence why custom check prefixes can contain them).

Have you seen any examples of typos where the problem was a leading underscore/dash/digit?

if (!Validator.match(Prefix)) {
errs() << "error: supplied " << Kind << " prefix must start with a "
<< "letter and contain only alphanumeric characters, hyphens, and "
<< "underscores: '" << Prefix << "'\n";
<< "letter or digit and contain only ascii alphanumeric "
<< "characters, hyphens, and underscores: '" << Prefix << "'\n";
return false;
}
if (!UniquePrefixes.insert(Prefix).second) {
errs() << "error: supplied " << Kind << " prefix must be unique among "
<< "check and comment prefixes: '" << Prefix << "'\n";
return false;
}
for (StringRef Directive : Suffixes) {
if (Prefix.ends_with(Directive)) {
errs() << "error: supplied " << Kind << " prefix must not end with "
<< "directive: '" << Directive << "', prefix: '" << Prefix
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the variable name and message here to use "suffix", per the previous request.

<< "'\n";
return false;
}
}
}
return true;
}
Expand Down
14 changes: 7 additions & 7 deletions llvm/test/FileCheck/comment/bad-comment-prefix.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@
# Check empty comment prefix.
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
RUN: -comment-prefixes= | \
RUN: FileCheck -check-prefix=PREFIX-EMPTY %s
RUN: FileCheck -check-prefix=EMPTY-PREFIX %s
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
RUN: -comment-prefixes=,FOO | \
RUN: FileCheck -check-prefix=PREFIX-EMPTY %s
RUN: FileCheck -check-prefix=EMPTY-PREFIX %s
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
RUN: -comment-prefixes=FOO, | \
RUN: FileCheck -check-prefix=PREFIX-EMPTY %s
RUN: FileCheck -check-prefix=EMPTY-PREFIX %s
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
RUN: -comment-prefixes=FOO,,BAR | \
RUN: FileCheck -check-prefix=PREFIX-EMPTY %s
PREFIX-EMPTY: error: supplied comment prefix must not be the empty string
RUN: FileCheck -check-prefix=EMPTY-PREFIX %s
EMPTY-PREFIX: error: supplied comment prefix must not be the empty string

# Check invalid characters in comment prefix.
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
Expand All @@ -22,8 +22,8 @@ RUN: FileCheck -check-prefix=PREFIX-BAD-CHAR1 %s
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
RUN: -comment-prefixes='foo ' | \
RUN: FileCheck -check-prefix=PREFIX-BAD-CHAR2 %s
PREFIX-BAD-CHAR1: error: supplied comment prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: '.'
PREFIX-BAD-CHAR2: error: supplied comment prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'foo '
PREFIX-BAD-CHAR1: error: supplied comment prefix must start with a letter or digit and contain only ascii alphanumeric characters, hyphens, and underscores: '.'
PREFIX-BAD-CHAR2: error: supplied comment prefix must start with a letter or digit and contain only ascii alphanumeric characters, hyphens, and underscores: 'foo '

# Check duplicate comment prefixes.
RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
Expand Down
8 changes: 4 additions & 4 deletions llvm/test/FileCheck/comment/suffixes.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Comment prefixes plus check directive suffixes are not comment directives
# and are treated as plain text.
# Comment prefixes plus check directive suffixes are forbidden.
# FIXME: currently not verified bq ValidatePrefixes missing defaulted comment prefixes?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There still some unresolved error, working on it

# note: command had no output on stdout or stderr
# error: command failed with exit status: 2


RUN: echo foo > %t.in
RUN: echo bar >> %t.in
Expand All @@ -12,11 +12,11 @@ RUN: FileCheck -check-prefix=CHECK1 %s
CHECK1: .chk:1:18: remark: CHECK: expected string found in input
CHECK1: .chk:2:17: remark: CHECK: expected string found in input

# But we can define them as comment prefixes.
# But we can define them as comment prefixes; still forbidden.

RUN: %ProtectFileCheckOutput \
RUN: FileCheck -dump-input=never -vv -comment-prefixes=COM,RUN,RUN-NOT %t.chk < %t.in 2>&1 | \
RUN: FileCheck -check-prefix=CHECK2 %s

CHECK2: .chk:1:18: remark: CHECK: expected string found in input
CHECK2: error: supplied comment prefix must not end with directive: '-NOT', prefix: 'RUN-NOT'
CHECK2-NOT: .chk:2
16 changes: 8 additions & 8 deletions llvm/test/FileCheck/numeric-expression.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,16 +593,16 @@ CALL-MISSING-ARGUMENT-MSG-NEXT: {{C}}ALL-MISSING-ARGUMENT-NEXT: {{\[\[#add\(NUMV
CALL-MISSING-ARGUMENT-MSG-NEXT: {{^}} ^{{$}}

RUN: %ProtectFileCheckOutput \
RUN: not FileCheck -D#NUMVAR=10 --check-prefix CALL-WRONG-ARGUMENT-COUNT --input-file %s %s 2>&1 \
RUN: | FileCheck --strict-whitespace --check-prefix CALL-WRONG-ARGUMENT-COUNT-MSG %s
RUN: not FileCheck -D#NUMVAR=10 --check-prefix CALL-WRONG-ARGUMENT-NUM --input-file %s %s 2>&1 \
RUN: | FileCheck --strict-whitespace --check-prefix CALL-WRONG-ARGUMENT-NUM-MSG %s

CALL WRONG ARGUMENT COUNT
CALL WRONG ARGUMENT NUM
30
CALL-WRONG-ARGUMENT-COUNT-LABEL: CALL WRONG ARGUMENT COUNT
CALL-WRONG-ARGUMENT-COUNT-NEXT: [[#add(NUMVAR)]]
CALL-WRONG-ARGUMENT-COUNT-MSG: numeric-expression.txt:[[#@LINE-1]]:36: error: function 'add' takes 2 arguments but 1 given
CALL-WRONG-ARGUMENT-COUNT-MSG-NEXT: {{C}}ALL-WRONG-ARGUMENT-COUNT-NEXT: {{\[\[#add\(NUMVAR\)\]\]}}
CALL-WRONG-ARGUMENT-COUNT-MSG-NEXT: {{^}} ^{{$}}
CALL-WRONG-ARGUMENT-NUM-LABEL: CALL WRONG ARGUMENT NUM
CALL-WRONG-ARGUMENT-NUM-NEXT: [[#add(NUMVAR)]]
CALL-WRONG-ARGUMENT-NUM-MSG: numeric-expression.txt:[[#@LINE-1]]:34: error: function 'add' takes 2 arguments but 1 given
CALL-WRONG-ARGUMENT-NUM-MSG-NEXT: {{C}}ALL-WRONG-ARGUMENT-NUM-NEXT: {{\[\[#add\(NUMVAR\)\]\]}}
CALL-WRONG-ARGUMENT-NUM-MSG-NEXT: {{^}} ^{{$}}

RUN: %ProtectFileCheckOutput \
RUN: not FileCheck -D#NUMVAR=10 --check-prefix CALL-UNDEFINED-FUNCTION --input-file %s %s 2>&1 \
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/FileCheck/validate-check-prefix.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
foobar
; A1a-B_c: foobar

; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
; BAD_PREFIX: supplied check prefix must start with a letter or digit and contain only ascii alphanumeric characters, hyphens, and underscores: 'A!'

; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check and comment prefixes: 'REPEAT'

Expand Down
Loading