compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes #134463

jieyouxu · 2024-12-18T12:30:47Z

This was fragile as it was based on host target passed to compiletest,
but the user could cross-compile and run test for a different target
(e.g. cross from linux to msvc, but msvc won't be set on the target).
Furthermore, it was also very surprising as normally revision names
(other than CHECK) was accepted as FileCheck prefixes.

This partially reverts the MSVC/NONMSVC predefined FileCheck
prefix registration introduced historically for some codegen tests.

This makes some codegen tests more verbose since they now need to
explicitly introduce MSVC/NONMSVC revisions, but I think that's
less surprising, e.g.:

//@ revisions: MSVC NONMSVC
//@[MSVC] only-msvc
//@[NONMSVC] ignore-msvc

Note that revisions are not only FileCheck prefixes in
FileCheck-based test suites, as they also can be used
to conditionally apply certain compiletest directives.

r? @Zalathar (or reroll a r/? compiletest reviewer)

try-job: x86_64-msvc
try-job: i686-msvc
try-job: x86_64-mingw-1
try-job: i686-mingw

jieyouxu · 2024-12-18T12:32:14Z

@bors try

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes This was fragile as it was based on host target passed to compiletest, but the user could cross-compile and run test for a different target (e.g. cross from linux to msvc, but msvc won't be set on the target). Furthermore, it was also very surprising as normally revision names (other than `CHECK`) was accepted as FileCheck prefixes. This partially reverts the `MSVC`/`NONMSVC` predefined FileCheck prefix registration introduced in rust-lang#120656 for some codegen tests. This makes some codegen tests more verbose since they now need to explicitly introduce `MSVC`/`NONMSVC` revisions, but I think that's less surprising, e.g.: ```rs //@ revisions: MSVC NONMSVC //`@[MSVC]` only-msvc //`@[NONMSVC]` ignore-msvc ``` Note that revisions are not *only* FileCheck prefixes in FileCheck-based test suites, as they also can be used to conditionally apply certain compiletest directives. r? `@Zalathar` (or reroll a `r/? compiletest` reviewer) try-job: x86_64-msvc try-job: i686-msvc try-job: x86_64-mingw try-job: i686-mingw

bors · 2024-12-18T12:33:25Z

⌛ Trying commit 2b41c9d with merge 48416db...

Zalathar · 2024-12-18T12:49:18Z

src/tools/compiletest/src/runtest.rs

+        // Tests are allowed to use a revision name as a check prefix.
        if let Some(rev) = self.revision {
            filecheck.arg("--check-prefix").arg(rev);
        }

-        // Some tests also expect either the MSVC or NONMSVC prefix to be defined.
-        let msvc_or_not = if self.config.target.contains("msvc") { "MSVC" } else { "NONMSVC" };
-        filecheck.arg("--check-prefix").arg(msvc_or_not);
-
-        // The filecheck tool normally fails if a prefix is defined but not used.
-        // However, we define several prefixes globally for all tests.
+        // The filecheck tool normally fails if a prefix is defined but not used. However, sometimes
+        // revisions are used to specify *compiletest* directives which are not FileCheck concerns.


Nitpick: If we're revising these comments, I think they should acknowledge that auto-registering revision names as check prefix is a bit sketchy, and that having to pass --allow-unused-prefix is an unfortunate side-effect of not knowing whether the test author actually wanted revision-specific check prefixes or not.

Good point, I'll update the docs here to better reflect the behavior after I try to run some test jobs.

The main reason I haven't tried to fix this yet is that there are quite a few tests affected, and in tests that use both CHECK and revision-specific prefixes, there's a risk of accidentally only checking the CHECK ones and silently ignoring all the revision ones.

True. I'll think about this a bit more.

The main reason I haven't tried to fix this yet is that there are quite a few tests affected, and in tests that use both CHECK and revision-specific prefixes, there's a risk of accidentally only checking the CHECK ones and silently ignoring all the revision ones.

Don't we register the revisions via

if let Some(rev) = self.revision { filecheck.arg("--check-prefix").arg(rev); }

?

EDIT: nvm I see what you mean -- we can't distinguish between whether the author wanted to:

Use revisions but ONLY for compiletest directive purposes.

Use revisions but ONLY for filecheck prefix purposes.

Use revisions for BOTH.

You are saying, that we should actually properly have an orthogonal set, something like //@ filecheck-prefixes: MSVC NONMSVC etc. right?

I added comments back + further clarified the current situation.

Opened #134510 to track this and backlinked from comments.

jieyouxu · 2024-12-18T12:56:55Z

@bors try

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes This was fragile as it was based on host target passed to compiletest, but the user could cross-compile and run test for a different target (e.g. cross from linux to msvc, but msvc won't be set on the target). Furthermore, it was also very surprising as normally revision names (other than `CHECK`) was accepted as FileCheck prefixes. This partially reverts the `MSVC`/`NONMSVC` predefined FileCheck prefix registration introduced in rust-lang#120656 for some codegen tests. This makes some codegen tests more verbose since they now need to explicitly introduce `MSVC`/`NONMSVC` revisions, but I think that's less surprising, e.g.: ```rs //@ revisions: MSVC NONMSVC //`@[MSVC]` only-msvc //`@[NONMSVC]` ignore-msvc ``` Note that revisions are not *only* FileCheck prefixes in FileCheck-based test suites, as they also can be used to conditionally apply certain compiletest directives. r? `@Zalathar` (or reroll a `r/? compiletest` reviewer) try-job: x86_64-msvc try-job: i686-msvc try-job: x86_64-mingw-1 try-job: i686-mingw

bors · 2024-12-18T12:58:13Z

⌛ Trying commit 2b41c9d with merge 1ec0d13...

bors · 2024-12-18T15:37:29Z

☀️ Try build successful - checks-actions
Build commit: 1ec0d13 (1ec0d13e414e1cfd2cc727c342eb86a7e0326e3a)

jieyouxu · 2024-12-19T12:03:46Z

Rebased to add clarifying comments as requested in #134463 (comment), no functional changes.

jieyouxu · 2024-12-19T12:10:03Z

@rustbot ready

src/tools/compiletest/src/runtest.rs

This was fragile as it was based on host target passed to compiletest, but the user could cross-compile and run test for a different target (e.g. cross from linux to msvc, but msvc won't be set on the target). Furthermore, it was also very surprising as normally revision names (other than `CHECK`) was accepted as FileCheck prefixes.

jieyouxu · 2024-12-19T12:37:47Z

Trimmed the comment down and just stole ur comment instead. The nuisance can be left to the discussions in the linked issue.

Zalathar · 2024-12-19T12:41:37Z

@bors r+

bors · 2024-12-19T12:41:39Z

📌 Commit 5415f06 has been approved by Zalathar

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#134463 (compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes) - rust-lang#134487 (Add reference annotations for the `coverage` attribute) - rust-lang#134497 (coverage: Store coverage source regions as `Span` until codegen (take 2)) - rust-lang#134502 (Update std libc version to 0.2.169) - rust-lang#134506 (Remove a duplicated check that doesn't do anything anymore.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134463 - jieyouxu:filecheck-prefix, r=Zalathar compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes This was fragile as it was based on host target passed to compiletest, but the user could cross-compile and run test for a different target (e.g. cross from linux to msvc, but msvc won't be set on the target). Furthermore, it was also very surprising as normally revision names (other than `CHECK`) was accepted as FileCheck prefixes. This partially reverts the `MSVC`/`NONMSVC` predefined FileCheck prefix registration introduced historically for some codegen tests. This makes some codegen tests more verbose since they now need to explicitly introduce `MSVC`/`NONMSVC` revisions, but I think that's less surprising, e.g.: ```rs //@ revisions: MSVC NONMSVC //`@[MSVC]` only-msvc //`@[NONMSVC]` ignore-msvc ``` Note that revisions are not *only* FileCheck prefixes in FileCheck-based test suites, as they also can be used to conditionally apply certain compiletest directives. r? `@Zalathar` (or reroll a `r/? compiletest` reviewer) try-job: x86_64-msvc try-job: i686-msvc try-job: x86_64-mingw-1 try-job: i686-mingw

jieyouxu added the A-compiletest Area: The compiletest test runner label Dec 18, 2024

rustbot assigned Zalathar Dec 18, 2024

jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2024

This comment has been minimized.

Sign in to view

jieyouxu mentioned this pull request Dec 18, 2024

compiletest: Deduplicate --check-prefix flags #134415

Closed

Zalathar reviewed Dec 18, 2024

View reviewed changes

jieyouxu force-pushed the filecheck-prefix branch from 2b41c9d to de837ce Compare December 19, 2024 12:03

jieyouxu mentioned this pull request Dec 19, 2024

compiletest: FileCheck-based tests conflate FileCheck prefixes with compiletest revisions #134510

Open

jieyouxu force-pushed the filecheck-prefix branch from de837ce to 3e2aa53 Compare December 19, 2024 12:09

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2024

Zalathar reviewed Dec 19, 2024

View reviewed changes

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

jieyouxu added 2 commits December 19, 2024 20:36

Explicitly register MSVC/NONMSVC revisions for some codegen tests

5415f06

jieyouxu force-pushed the filecheck-prefix branch from 3e2aa53 to 5415f06 Compare December 19, 2024 12:37

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2024

matthiaskrgr mentioned this pull request Dec 19, 2024

Rollup of 5 pull requests #134516

Merged

bors merged commit b978498 into rust-lang:master Dec 20, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 20, 2024

jieyouxu deleted the filecheck-prefix branch December 20, 2024 07:02

jieyouxu mentioned this pull request Dec 21, 2024

Windows x86: Change i128 to return via the vector ABI #134290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes #134463

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes #134463

jieyouxu commented Dec 18, 2024 •

edited

Loading

jieyouxu commented Dec 18, 2024

bors commented Dec 18, 2024

This comment has been minimized.

Zalathar Dec 18, 2024

jieyouxu Dec 18, 2024

Zalathar Dec 18, 2024

jieyouxu Dec 18, 2024

jieyouxu Dec 19, 2024 •

edited

Loading

jieyouxu Dec 19, 2024

jieyouxu Dec 19, 2024

jieyouxu commented Dec 18, 2024

bors commented Dec 18, 2024

bors commented Dec 18, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

Zalathar commented Dec 19, 2024

bors commented Dec 19, 2024

compiletest: don't register predefined MSVC/NONMSVC FileCheck prefixes #134463

compiletest: don't register predefined MSVC/NONMSVC FileCheck prefixes #134463

Conversation

jieyouxu commented Dec 18, 2024 • edited Loading

jieyouxu commented Dec 18, 2024

bors commented Dec 18, 2024

This comment has been minimized.

Zalathar Dec 18, 2024

Choose a reason for hiding this comment

jieyouxu Dec 18, 2024

Choose a reason for hiding this comment

Zalathar Dec 18, 2024

Choose a reason for hiding this comment

jieyouxu Dec 18, 2024

Choose a reason for hiding this comment

jieyouxu Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Dec 19, 2024

Choose a reason for hiding this comment

jieyouxu Dec 19, 2024

Choose a reason for hiding this comment

jieyouxu commented Dec 18, 2024

bors commented Dec 18, 2024

bors commented Dec 18, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

Zalathar commented Dec 19, 2024

bors commented Dec 19, 2024

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes #134463

compiletest: don't register predefined `MSVC`/`NONMSVC` FileCheck prefixes #134463

jieyouxu commented Dec 18, 2024 •

edited

Loading

jieyouxu Dec 19, 2024 •

edited

Loading