Skip to content

Commit fa6a18d

Browse files
committed
compiletest: don't register 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.
1 parent bab18a5 commit fa6a18d

File tree

2 files changed

+22
-17
lines changed

2 files changed

+22
-17
lines changed

src/tools/compiletest/src/runtest.rs

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1958,23 +1958,35 @@ impl<'test> TestCx<'test> {
19581958
let mut filecheck = Command::new(self.config.llvm_filecheck.as_ref().unwrap());
19591959
filecheck.arg("--input-file").arg(output).arg(&self.testpaths.file);
19601960

1961-
// FIXME: Consider making some of these prefix flags opt-in per test,
1962-
// via `filecheck-flags` or by adding new header directives.
1963-
19641961
// Because we use custom prefixes, we also have to register the default prefix.
19651962
filecheck.arg("--check-prefix=CHECK");
19661963

1967-
// Some tests use the current revision name as a check prefix.
1964+
// FIXME(#134510): the current scheme of conflating `compiletest` revisions with `FileCheck`
1965+
// prefixes is questionable. The current scheme means that `compiletest` revision names
1966+
// **implicitly** registers a `FileCheck` prefix of the same name. But this is questionable:
1967+
// with this setup, there is no way to distinguish whether the test writer wants to:
1968+
//
1969+
// 1. Use revisions but ONLY for compiletest directive purposes. E.g. `FileCheck`
1970+
// annotations should ONLY be using the default `CHECK` prefix.
1971+
// 2. Use revisions but ONLY for filecheck prefix purposes. This use case is somewhat
1972+
// questionable; previously we implicitly injected `MSVC` for msvc host and `NONMSVC` for
1973+
// non-msvc host, but that is problematic in cross-compile test scenarios.
1974+
// 3. Use revisions for BOTH. This conflates `compiletest` directives (e.g. conditional test
1975+
// execution) and `FileCheck` directives, and tthet test writer can't really separate
1976+
// them.
1977+
//
1978+
// A better solution might be to split these probably-orthogonal concepts, and use a
1979+
// dedicated directive like `//@ filecheck-prefixes: MSVC NONMSVC` to not conflate
1980+
// `compiletest` directives with `FileCheck` prefixes.
1981+
1982+
// HACK: tests are allowed to use a revision name as a check prefix.
19681983
if let Some(rev) = self.revision {
19691984
filecheck.arg("--check-prefix").arg(rev);
19701985
}
19711986

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

19801992
// Provide more context on failures.

tests/codegen/meta-filecheck/msvc-prefix-good.rs

Lines changed: 0 additions & 7 deletions
This file was deleted.

0 commit comments

Comments
 (0)