compiler-team icon indicating copy to clipboard operation
compiler-team copied to clipboard

Normalize FileCheck directives

Open jieyouxu opened this issue 1 year ago • 1 comments

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 https://github.com/rust-lang/rust/pull/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

  • We don't currently warn on FileCheck directives without a colon :: https://github.com/rust-lang/rust/issues/130981
  • We don't currently deny special FileCheck prefixes like CHECK: https://github.com/rust-lang/rust/issues/130982

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:

  • [x] 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 avatar Sep 28 '24 15:09 jieyouxu

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 avatar Sep 28 '24 15:09 rustbot

I'm going to close this as postpone, going to revisit this once the compiletest directive handling is less of a pain.

jieyouxu avatar Feb 28 '25 16:02 jieyouxu