Skip to content
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.

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

Normalize FileCheck directives #789

Open
1 of 3 tasks
jieyouxu opened this issue Sep 28, 2024 · 1 comment
Open
1 of 3 tasks

Normalize FileCheck directives #789

jieyouxu opened this issue Sep 28, 2024 · 1 comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@jieyouxu
Copy link
Member

Proposal

Background

LLVM FileCheck is a LLVM bin tool is that used in our
tests/{assembly,codegen} test suites. FileCheck directives are not handled
by compiletest (which uses //@ prefixes).

Currently, assembly and codegen tests ("tests") uses the following syntax:

Default FileCheck directives

Without specifying revisions, or in the case of revisions specified but you want
unconditionally checked FileCheck directives, FileCheck will look for COM
(which are comment directives that are ignored) and CHECK-prefixed directives
(default). Example matching directives:

// COM:
// CHECK:
// CHECK-NEXT:
// CHECK-SAME:
// CHECK-EMPTY:
// CHECK-NOT:
// CHECK-COUNT:
// CHECK-DAG:
// CHECK-LABEL:

Revisioned FileCheck directives

When you specify revisions, i.e. //@ revisions: a b c, compiletest will pass
the revision names to FileCheck as --check-prefixes=a,b,c, which will modify
how FileCheck matches directives. Namely, FileCheck will now look for
directives with matching prefixes in addition to the default CHECK prefix.
Example matching directives:

// COM:
// CHECK:         # <- universally matched: all be matched in each revision a, b, c
// a:             # <- only checked for test revision a
// b-SAME:        # <- only checked for test revision b

Proposed changes

  1. Normalize anything that uses an invalid prefix (e.g. FIXME-CHECK:) to
    COM: so we can eventually check for invalid prefixes.
  2. Decide on if we want to enforce casing of revisions and FileCheck prefixes,
    e.g. x86_64-NOT: vs X86_64-NOT: or foo-not: vs FOO-NOT:. If so,
    enforce that they are used consistency.
  3. Decide on if we want to enforce FileCheck directive prefixes with CHECK-,
    i.e. instead of foo-NOT: we will always write CHECK-foo-NOT;. If so,
    enforce it.

@tgross35 has a prototype PR at rust-lang/rust#128018
for how some of the changes might look like (subject to changes as the
unresolved questions getting resolved).

Unresolved questions

  • Should FileCheck prefixes be always capitalized regardless of revision
    name capitalization?
  • Should we make sure revision names (in test suites that uses FileCheck) always
    be uppercase? Always lowercase?
  • Should FileCheck prefixes always begin with CHECK? I.e. if we have a
    revisions: foo, instead of foo: or foo-NOT: we would have CHECK-foo:
    and CHECK-foo-NOT:.

Remarks

Mentors or Reviewers

Anyone who writes a lot of assembly and codegen tests for UX feedback in both
reading and writing. cc @scottmcm, @workingjubilee, @saethlin, @RalfJung,
@DianQK, @nikic, @rust-lang/wg-llvm for advice and preferences.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can
    second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are
      proposing a new public-facing feature, such as a -C flag, then full team
      check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on
      either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections
    are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip
stream for that. Use this issue to leave procedural comments, such as
volunteering to review, indicating that you second the proposal (or third, etc),
or raising a concern that you would like to be addressed.

@jieyouxu jieyouxu added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Sep 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2024

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Sep 28, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

3 participants