From 4ae22fba5b4af7ffa223c46e7d81a15f53a6a387 Mon Sep 17 00:00:00 2001 From: Folkert de Vries Date: Fri, 4 Jul 2025 20:10:53 +0200 Subject: [PATCH 1/3] move the `va_copy`, `va_arg` and `va_end` to `core::intrinsics` --- library/core/src/ffi/va_list.rs | 18 +----------------- library/core/src/intrinsics/mod.rs | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/library/core/src/ffi/va_list.rs b/library/core/src/ffi/va_list.rs index 8f7c090bc1ba2..88ad11977774d 100644 --- a/library/core/src/ffi/va_list.rs +++ b/library/core/src/ffi/va_list.rs @@ -5,6 +5,7 @@ use crate::ffi::c_void; #[allow(unused_imports)] use crate::fmt; +use crate::intrinsics::{va_arg, va_copy, va_end}; use crate::marker::{PhantomData, PhantomInvariantLifetime}; use crate::ops::{Deref, DerefMut}; @@ -280,20 +281,3 @@ impl<'f> Drop for VaListImpl<'f> { // This works for now, since `va_end` is a no-op on all current LLVM targets. } } - -/// Destroy the arglist `ap` after initialization with `va_start` or -/// `va_copy`. -#[rustc_intrinsic] -#[rustc_nounwind] -unsafe fn va_end(ap: &mut VaListImpl<'_>); - -/// Copies the current location of arglist `src` to the arglist `dst`. -#[rustc_intrinsic] -#[rustc_nounwind] -unsafe fn va_copy<'f>(dest: *mut VaListImpl<'f>, src: &VaListImpl<'f>); - -/// Loads an argument of type `T` from the `va_list` `ap` and increment the -/// argument `ap` points to. -#[rustc_intrinsic] -#[rustc_nounwind] -unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> T; diff --git a/library/core/src/intrinsics/mod.rs b/library/core/src/intrinsics/mod.rs index ab99492638ec0..791d10eda6d0d 100644 --- a/library/core/src/intrinsics/mod.rs +++ b/library/core/src/intrinsics/mod.rs @@ -54,6 +54,7 @@ )] #![allow(missing_docs)] +use crate::ffi::va_list::{VaArgSafe, VaListImpl}; use crate::marker::{ConstParamTy, DiscriminantKind, PointeeSized, Tuple}; use crate::ptr; @@ -3142,3 +3143,25 @@ pub(crate) const fn miri_promise_symbolic_alignment(ptr: *const (), align: usize } ) } + +/// Copies the current location of arglist `src` to the arglist `dst`. +/// +/// FIXME: document safety requirements +#[rustc_intrinsic] +#[rustc_nounwind] +pub unsafe fn va_copy<'f>(dest: *mut VaListImpl<'f>, src: &VaListImpl<'f>); + +/// Loads an argument of type `T` from the `va_list` `ap` and increment the +/// argument `ap` points to. +/// +/// FIXME: document safety requirements +#[rustc_intrinsic] +#[rustc_nounwind] +pub unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> T; + +/// Destroy the arglist `ap` after initialization with `va_start` or `va_copy`. +/// +/// FIXME: document safety requirements +#[rustc_intrinsic] +#[rustc_nounwind] +pub unsafe fn va_end(ap: &mut VaListImpl<'_>); From 7405e2a9154268955de216840ef4dd7e975b0cd5 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Fri, 4 Jul 2025 19:20:09 +0800 Subject: [PATCH 2/3] Improve compiletest config documentation Including a bunch of FIXMEs. --- src/tools/compiletest/src/common.rs | 395 ++++++++++++++---- src/tools/compiletest/src/debuggers.rs | 2 + src/tools/compiletest/src/executor.rs | 12 +- src/tools/compiletest/src/lib.rs | 11 + src/tools/compiletest/src/runtest.rs | 22 +- .../compiletest/src/runtest/debuginfo.rs | 4 + 6 files changed, 353 insertions(+), 93 deletions(-) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index cdce5d941d015..7122746fa87eb 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -172,207 +172,422 @@ pub enum Sanitizer { Hwaddress, } -/// Configuration for compiletest +/// Configuration for `compiletest` *per invocation*. +/// +/// In terms of `bootstrap`, this means that `./x test tests/ui tests/run-make` actually correspond +/// to *two* separate invocations of `compiletest`. +/// +/// FIXME: this `Config` struct should be broken up into smaller logically contained sub-config +/// structs, it's too much of a "soup" of everything at the moment. +/// +/// # Configuration sources +/// +/// Configuration values for `compiletest` comes from several sources: +/// +/// - CLI args passed from `bootstrap` while running the `compiletest` binary. +/// - Env vars. +/// - Discovery (e.g. trying to identify a suitable debugger based on filesystem discovery). +/// - Cached output of running the `rustc` under test (e.g. output of `rustc` print requests). +/// +/// FIXME: make sure we *clearly* account for sources of *all* config options. +/// +/// FIXME: audit these options to make sure we are not hashing less than necessary for build stamp +/// (for changed test detection). #[derive(Debug, Default, Clone)] pub struct Config { - /// `true` to overwrite stderr/stdout files instead of complaining about changes in output. + /// Some test [`Mode`]s support [snapshot testing], where a *reference snapshot* of outputs (of + /// `stdout`, `stderr`, or other form of artifacts) can be compared to the *actual output*. + /// + /// This option can be set to `true` to update the *reference snapshots* in-place, otherwise + /// `compiletest` will only try to compare. + /// + /// [snapshot testing]: https://jestjs.io/docs/snapshot-testing pub bless: bool, - /// Stop as soon as possible after any test fails. - /// May run a few more tests before stopping, due to threading. + /// Attempt to stop as soon as possible after any test fails. We may still run a few more tests + /// before stopping when multiple test threads are used. pub fail_fast: bool, - /// The library paths required for running the compiler. + /// Path to libraries needed to run the *staged* `rustc`-under-test on the **host** platform. + /// + /// FIXME: maybe rename this to reflect (1) which target platform (host, not target), and (2) + /// which `rustc` (the `rustc`-under-test, not the stage 0 `rustc` unless forced). pub compile_lib_path: Utf8PathBuf, - /// The library paths required for running compiled programs. + /// Path to libraries needed to run the compiled executable for the **target** platform. This + /// corresponds to the **target** sysroot libraries, including the **target** standard library. + /// + /// FIXME: maybe rename this to reflect (1) which target platform (target, not host), and (2) + /// what "run libraries" are against. + /// + /// FIXME: this is very under-documented in conjunction with the `remote-test-client` scheme and + /// `RUNNER` scheme to actually run the target executable under the target platform environment, + /// cf. [`Self::remote_test_client`] and [`Self::runner`]. pub run_lib_path: Utf8PathBuf, - /// The rustc executable. + /// Path to the *staged* `rustc`-under-test. Unless forced, this `rustc` is *staged*, and must + /// not be confused with [`Self::stage0_rustc_path`]. + /// + /// FIXME: maybe rename this to reflect that this is the `rustc`-under-test. pub rustc_path: Utf8PathBuf, - /// The cargo executable. + /// Path to a *staged* **host** platform cargo executable (unless stage 0 is forced). This + /// staged `cargo` is only used within `run-make` test recipes during recipe run time (and is + /// *not* used to compile the test recipes), and so must be staged as there may be differences + /// between e.g. beta `cargo` vs in-tree `cargo`. + /// + /// FIXME: maybe rename this to reflect that this is a *staged* host cargo. + /// + /// FIXME(#134109): split `run-make` into two test suites, a test suite *with* staged cargo, and + /// another test suite *without*. pub cargo_path: Option, - /// Rustc executable used to compile run-make recipes. + /// Path to the stage 0 `rustc` used to build `run-make` recipes. This must not be confused with + /// [`Self::rustc_path`]. pub stage0_rustc_path: Option, - /// The rustdoc executable. + /// Path to the `rustdoc`-under-test. Like [`Self::rustc_path`], this `rustdoc` is *staged*. pub rustdoc_path: Option, - /// The coverage-dump executable. + /// Path to the `src/tools/coverage-dump/` bootstrap tool executable. pub coverage_dump_path: Option, - /// The Python executable to use for LLDB and htmldocck. + /// Path to the Python 3 executable to use for LLDB and htmldocck. + /// + /// FIXME: the `lldb` setup currently requires I believe Python 3.10 **exactly**, it can't even + /// be Python 3.11 or 3.9... pub python: String, - /// The jsondocck executable. + /// Path to the `src/tools/jsondocck/` bootstrap tool executable. pub jsondocck_path: Option, - /// The jsondoclint executable. + /// Path to the `src/tools/jsondoclint/` bootstrap tool executable. pub jsondoclint_path: Option, - /// The LLVM `FileCheck` binary path. + /// Path to a host LLVM `FileCheck` executable. pub llvm_filecheck: Option, - /// Path to LLVM's bin directory. + /// Path to a host LLVM bintools directory. pub llvm_bin_dir: Option, - /// The path to the Clang executable to run Clang-based tests with. If - /// `None` then these tests will be ignored. + /// The path to the **target** `clang` executable to run `clang`-based tests with. If `None`, + /// then these tests will be ignored. pub run_clang_based_tests_with: Option, - /// The directory containing the sources. + /// Path to the directory containing the sources. This corresponds to the root folder of a + /// `rust-lang/rust` checkout. + /// + /// FIXME: this name is confusing, because this is actually `$checkout_root`, **not** the + /// `$checkout_root/src/` folder. pub src_root: Utf8PathBuf, - /// The directory containing the test suite sources. Must be a subdirectory of `src_root`. + + /// Path to the directory containing the test suites sources. This corresponds to the + /// `$src_root/tests/` folder. + /// + /// Must be an immediate subdirectory of [`Self::src_root`]. + /// + /// FIXME: this name is also confusing, maybe just call it `tests_root`. pub src_test_suite_root: Utf8PathBuf, - /// Root build directory (e.g. `build/`). + /// Path to the build directory (e.g. `build/`). pub build_root: Utf8PathBuf, - /// Test suite specific build directory (e.g. `build/host/test/ui/`). + + /// Path to the test suite specific build directory (e.g. `build/host/test/ui/`). + /// + /// Must be a subdirectory of [`Self::build_root`]. pub build_test_suite_root: Utf8PathBuf, - /// The directory containing the compiler sysroot + /// Path to the directory containing the sysroot of the `rustc`-under-test. + /// + /// When stage 0 is forced, this will correspond to the sysroot *of* that specified stage 0 + /// `rustc`. + /// + /// FIXME: this name is confusing, because it doesn't specify *which* compiler this sysroot + /// corresponds to. It's actually the `rustc`-under-test, and not the bootstrap `rustc`, unless + /// stage 0 is forced and no custom stage 0 `rustc` was otherwise specified (so that it + /// *happens* to run against the bootstrap `rustc`, but this non-custom bootstrap `rustc` case + /// is not really supported). pub sysroot_base: Utf8PathBuf, /// The number of the stage under test. pub stage: u32, + /// The id of the stage under test (stage1-xxx, etc). + /// + /// FIXME: reconsider this string; this is hashed for test build stamp. pub stage_id: String, - /// The test mode, e.g. ui or debuginfo. + /// The test [`Mode`]. E.g. [`Mode::Ui`]. Each test mode can correspond to one or more test + /// suites. + /// + /// FIXME: stop using stringly-typed test suites! pub mode: Mode, - /// The test suite (essentially which directory is running, but without the - /// directory prefix such as tests) + /// The test suite. + /// + /// Example: `tests/ui/` is the "UI" test *suite*, which happens to also be of the [`Mode::Ui`] + /// test *mode*. + /// + /// Note that the same test directory (e.g. `tests/coverage/`) may correspond to multiple test + /// modes, e.g. `tests/coverage/` can be run under both [`Mode::CoverageRun`] and + /// [`Mode::CoverageMap`]. + /// + /// FIXME: stop using stringly-typed test suites! pub suite: String, - /// The debugger to use in debuginfo mode. Unset otherwise. + /// When specified, **only** the specified [`Debugger`] will be used to run against the + /// `tests/debuginfo` test suite. When unspecified, `compiletest` will attempt to find all three + /// of {`lldb`, `cdb`, `gdb`} implicitly, and then try to run the `debuginfo` test suite against + /// all three debuggers. + /// + /// FIXME: this implicit behavior is really nasty, in that it makes it hard for the user to + /// control *which* debugger(s) are available and used to run the debuginfo test suite. We + /// should have `bootstrap` allow the user to *explicitly* configure the debuggers, and *not* + /// try to implicitly discover some random debugger from the user environment. This makes the + /// debuginfo test suite particularly hard to work with. pub debugger: Option, - /// Run ignored tests + /// Run ignored tests *unconditionally*, overriding their ignore reason. + /// + /// FIXME: this is wired up through the test execution logic, but **not** accessible from + /// `bootstrap` directly; `compiletest` exposes this as `--ignored`. I.e. you'd have to use `./x + /// test $test_suite -- --ignored=true`. pub run_ignored: bool, - /// Whether rustc was built with debug assertions. + /// Whether *staged* `rustc`-under-test was built with debug assertions. + /// + /// FIXME: make it clearer that this refers to the staged `rustc`-under-test, not stage 0 + /// `rustc`. pub with_rustc_debug_assertions: bool, - /// Whether std was built with debug assertions. + /// Whether *staged* `std` was built with debug assertions. + /// + /// FIXME: make it clearer that this refers to the staged `std`, not stage 0 `std`. pub with_std_debug_assertions: bool, - /// Only run tests that match these filters + /// Only run tests that match these filters (using `libtest` "test name contains" filter logic). + /// + /// FIXME(#139660): the current hand-rolled test executor intentionally mimics the `libtest` + /// "test name contains" filter matching logic to preserve previous `libtest` executor behavior, + /// but this is often not intuitive. We should consider changing that behavior with an MCP to do + /// test path *prefix* matching which better corresponds to how `compiletest` `tests/` are + /// organized, and how users would intuitively expect the filtering logic to work like. pub filters: Vec, - /// Skip tests matching these substrings. Corresponds to - /// `test::TestOpts::skip`. `filter_exact` does not apply to these flags. + /// Skip tests matching these substrings. The matching logic exactly corresponds to + /// [`Self::filters`] but inverted. + /// + /// FIXME(#139660): ditto on test matching behavior. pub skip: Vec, - /// Exactly match the filter, rather than a substring + /// Exactly match the filter, rather than a substring. + /// + /// FIXME(#139660): ditto on test matching behavior. pub filter_exact: bool, - /// Force the pass mode of a check/build/run-pass test to this mode. + /// Force the pass mode of a check/build/run test to instead use this mode instead. + /// + /// FIXME: make it even more obvious (especially in PR CI where `--pass=check` is used) when a + /// pass mode is forced when the test fails, because it can be very non-obvious when e.g. an + /// error is emitted only when `//@ build-pass` but not `//@ check-pass`. pub force_pass_mode: Option, - /// Explicitly enable or disable running. + /// Explicitly enable or disable running of the target test binary. + /// + /// FIXME: this scheme is a bit confusing, and at times questionable. Re-evaluate this run + /// scheme. + /// + /// FIXME: Currently `--run` is a tri-state, it can be `--run={auto,always,never}`, and when + /// `--run=auto` is specified, it's run if the platform doesn't end with `-fuchsia`. See + /// [`Config::run_enabled`]. pub run: Option, - /// A command line to prefix program execution with, - /// for running under valgrind for example. + /// A command line to prefix target program execution with, for running under valgrind for + /// example, i.e. `$runner target.exe [args..]`. Similar to `CARGO_*_RUNNER` configuration. + /// + /// Note: this is not to be confused with [`Self::remote_test_client`], which is a different + /// scheme. /// - /// Similar to `CARGO_*_RUNNER` configuration. + /// FIXME: the runner scheme is very under-documented. pub runner: Option, - /// Flags to pass to the compiler when building for the host + /// Compiler flags to pass to the *staged* `rustc`-under-test when building for the **host** + /// platform. pub host_rustcflags: Vec, - /// Flags to pass to the compiler when building for the target + /// Compiler flags to pass to the *staged* `rustc`-under-test when building for the **target** + /// platform. pub target_rustcflags: Vec, - /// Whether the compiler and stdlib has been built with randomized struct layouts + /// Whether the *staged* `rustc`-under-test and the associated *staged* `std` has been built + /// with randomized struct layouts. pub rust_randomized_layout: bool, - /// Whether tests should be optimized by default. Individual test-suites and test files may - /// override this setting. + /// Whether tests should be optimized by default (`-O`). Individual test suites and test files + /// may override this setting. + /// + /// FIXME: this flag / config option is somewhat misleading. For instance, in ui tests, it's + /// *only* applied to the [`PassMode::Run`] test crate and not its auxiliaries. pub optimize_tests: bool, - /// Target system to be tested + /// Target platform tuple. pub target: String, - /// Host triple for the compiler being invoked + /// Host platform tuple. pub host: String, - /// Path to / name of the Microsoft Console Debugger (CDB) executable + /// Path to / name of the Microsoft Console Debugger (CDB) executable. + /// + /// FIXME: this is an *opt-in* "override" option. When this isn't provided, we try to conjure a + /// cdb by looking at the user's program files on Windows... See `debuggers::find_cdb`. pub cdb: Option, - /// Version of CDB + /// Version of CDB. + /// + /// FIXME: `cdb_version` is *derived* from cdb, but it's *not* technically a config! + /// + /// FIXME: audit cdb version gating. pub cdb_version: Option<[u16; 4]>, - /// Path to / name of the GDB executable + /// Path to / name of the GDB executable. + /// + /// FIXME: the fallback path when `gdb` isn't provided tries to find *a* `gdb` or `gdb.exe` from + /// `PATH`, which is... arguably questionable. + /// + /// FIXME: we are propagating a python from `PYTHONPATH`, not from an explicit config for gdb + /// debugger script. pub gdb: Option, /// Version of GDB, encoded as ((major * 1000) + minor) * 1000 + patch + /// + /// FIXME: this gdb version gating scheme is possibly questionable -- gdb does not use semver, + /// only its major version is likely materially meaningful, cf. + /// . Even the major version I'm not sure + /// is super meaningful. Maybe min gdb `major.minor` version gating is sufficient for the + /// purposes of debuginfo tests? + /// + /// FIXME: `gdb_version` is *derived* from gdb, but it's *not* technically a config! pub gdb_version: Option, - /// Version of LLDB + /// Version of LLDB. + /// + /// FIXME: `lldb_version` is *derived* from lldb, but it's *not* technically a config! pub lldb_version: Option, - /// Version of LLVM + /// Version of LLVM. + /// + /// FIXME: Audit the fallback derivation of + /// [`crate::directives::extract_llvm_version_from_binary`], that seems very questionable? pub llvm_version: Option, - /// Is LLVM a system LLVM + /// Is LLVM a system LLVM. pub system_llvm: bool, - /// Path to the android tools + /// Path to the android tools. + /// + /// Note: this is only used for android gdb debugger script in the debuginfo test suite. + /// + /// FIXME: take a look at this; this is piggy-backing off of gdb code paths but only for + /// `arm-linux-androideabi` target. pub android_cross_path: Utf8PathBuf, - /// Extra parameter to run adb on arm-linux-androideabi + /// Extra parameter to run adb on `arm-linux-androideabi`. + /// + /// FIXME: is this *only* `arm-linux-androideabi`, or is it also for other Tier 2/3 android + /// targets? + /// + /// FIXME: take a look at this; this is piggy-backing off of gdb code paths but only for + /// `arm-linux-androideabi` target. pub adb_path: String, - /// Extra parameter to run test suite on arm-linux-androideabi + /// Extra parameter to run test suite on `arm-linux-androideabi`. + /// + /// FIXME: is this *only* `arm-linux-androideabi`, or is it also for other Tier 2/3 android + /// targets? + /// + /// FIXME: take a look at this; this is piggy-backing off of gdb code paths but only for + /// `arm-linux-androideabi` target. pub adb_test_dir: String, - /// status whether android device available or not + /// Status whether android device available or not. When unavailable, this will cause tests to + /// panic when the test binary is attempted to be run. + /// + /// FIXME: take a look at this; this also influences adb in gdb code paths in a strange way. pub adb_device_status: bool, - /// the path containing LLDB's Python module + /// Path containing LLDB's Python module. + /// + /// FIXME: `PYTHONPATH` takes precedence over this flag...? See `runtest::run_lldb`. pub lldb_python_dir: Option, - /// Explain what's going on + /// Verbose dump a lot of info. + /// + /// FIXME: this is *way* too coarse; the user can't select *which* info to verbosely dump. pub verbose: bool, - /// Print one character per test instead of one line + /// (Useless) Adjust libtest output format. + /// + /// FIXME: the hand-rolled executor does not support non-JSON output, because `compiletest` need + /// to package test outcome as `libtest`-esque JSON that `bootstrap` can intercept *anyway*. + /// However, now that we don't use the `libtest` executor, this is useless. pub format: OutputFormat, - /// Whether to use colors in test. + /// Whether to use colors in test output. + /// + /// Note: the exact control mechanism is delegated to [`colored`]. pub color: ColorConfig, - /// where to find the remote test client process, if we're using it + /// Where to find the remote test client process, if we're using it. + /// + /// Note: this is *only* used for target platform executables created by `run-make` test + /// recipes. + /// + /// Note: this is not to be confused with [`Self::runner`], which is a different scheme. + /// + /// FIXME: the `remote_test_client` scheme is very under-documented. pub remote_test_client: Option, - /// mode describing what file the actual ui output will be compared to + /// [`CompareMode`] describing what file the actual ui output will be compared to. + /// + /// FIXME: currently, [`CompareMode`] is a mishmash of lot of things (different borrow-checker + /// model, different trait solver, different debugger, etc.). pub compare_mode: Option, /// If true, this will generate a coverage file with UI test files that run `MachineApplicable` /// diagnostics but are missing `run-rustfix` annotations. The generated coverage file is - /// created in `/rustfix_missing_coverage.txt` + /// created in `$test_suite_build_root/rustfix_missing_coverage.txt` pub rustfix_coverage: bool, - /// whether to run `tidy` (html-tidy) when a rustdoc test fails + /// Whether to run `tidy` (html-tidy) when a rustdoc test fails. pub has_html_tidy: bool, - /// whether to run `enzyme` autodiff tests + /// Whether to run `enzyme` autodiff tests. pub has_enzyme: bool, - /// The current Rust channel + /// The current Rust channel info. + /// + /// FIXME: treat this more carefully; "stable", "beta" and "nightly" are definitely valid, but + /// channel might also be "dev" or such, which should be treated as "nightly". pub channel: String, - /// Whether adding git commit information such as the commit hash has been enabled for building + /// Whether adding git commit information such as the commit hash has been enabled for building. + /// + /// FIXME: `compiletest` cannot trust `bootstrap` for this information, because `bootstrap` can + /// have bugs and had bugs on that logic. We need to figure out how to obtain this e.g. directly + /// from CI or via git locally. pub git_hash: bool, - /// The default Rust edition + /// The default Rust edition. + /// + /// FIXME: perform stronger validation for this. There are editions that *definitely* exists, + /// but there might also be "future" edition. pub edition: Option, - // Configuration for various run-make tests frobbing things like C compilers - // or querying about various LLVM component information. + // Configuration for various run-make tests frobbing things like C compilers or querying about + // various LLVM component information. + // + // FIXME: this really should be better packaged together. + // FIXME: these need better docs, e.g. for *host*, or for *target*? pub cc: String, pub cxx: String, pub cflags: String, @@ -382,41 +597,63 @@ pub struct Config { pub host_linker: Option, pub llvm_components: String, - /// Path to a NodeJS executable. Used for JS doctests, emscripten and WASM tests + /// Path to a NodeJS executable. Used for JS doctests, emscripten and WASM tests. pub nodejs: Option, - /// Path to a npm executable. Used for rustdoc GUI tests + /// Path to a npm executable. Used for rustdoc GUI tests. pub npm: Option, /// Whether to rerun tests even if the inputs are unchanged. pub force_rerun: bool, - /// Only rerun the tests that result has been modified according to Git status + /// Only rerun the tests that result has been modified according to `git status`. + /// + /// FIXME: this is undocumented. + /// + /// FIXME: how does this interact with [`Self::force_rerun`]? pub only_modified: bool, + // FIXME: these are really not "config"s, but rather are information derived from + // `rustc`-under-test. This poses an interesting conundrum: if we're testing the + // `rustc`-under-test, can we trust its print request outputs and target cfgs? In theory, this + // itself can break or be unreliable -- ideally, we'd be sharing these kind of information not + // through `rustc`-under-test's execution output. In practice, however, print requests are very + // unlikely to completely break (we also have snapshot ui tests for them). Furthermore, even if + // we share them via some kind of static config, that static config can still be wrong! Who + // tests the tester? Therefore, we make a pragmatic compromise here, and use information derived + // from print requests produced by the `rustc`-under-test. + // + // FIXME: move them out from `Config`, because they are *not* configs. pub target_cfgs: OnceLock, pub builtin_cfg_names: OnceLock>, pub supported_crate_types: OnceLock>, + /// FIXME: this is why we still need to depend on *staged* `std`, it's because we currently rely + /// on `#![feature(internal_output_capture)]` for [`std::io::set_output_capture`] to implement + /// `libtest`-esque `--no-capture`. + /// + /// FIXME: rename this to the more canonical `no_capture`, or better, invert this to `capture` + /// to avoid `!nocapture` double-negatives. pub nocapture: bool, - // Needed both to construct build_helper::git::GitConfig + /// Needed both to construct [`build_helper::git::GitConfig`]. pub nightly_branch: String, pub git_merge_commit_email: String, - /// True if the profiler runtime is enabled for this target. - /// Used by the "needs-profiler-runtime" directive in test files. + /// True if the profiler runtime is enabled for this target. Used by the + /// `needs-profiler-runtime` directive in test files. pub profiler_runtime: bool, /// Command for visual diff display, e.g. `diff-tool --color=always`. pub diff_command: Option, - /// Path to minicore aux library, used for `no_core` tests that need `core` stubs in - /// cross-compilation scenarios that do not otherwise want/need to `-Zbuild-std`. Used in e.g. - /// ABI tests. + /// Path to minicore aux library (`tests/auxiliary/minicore.rs`), used for `no_core` tests that + /// need `core` stubs in cross-compilation scenarios that do not otherwise want/need to + /// `-Zbuild-std`. Used in e.g. ABI tests. pub minicore_path: Utf8PathBuf, } impl Config { + /// FIXME: this run scheme is... confusing. pub fn run_enabled(&self) -> bool { self.run.unwrap_or_else(|| { // Auto-detect whether to run based on the platform. diff --git a/src/tools/compiletest/src/debuggers.rs b/src/tools/compiletest/src/debuggers.rs index c133d7fd4fbd0..0edc3d82d4f56 100644 --- a/src/tools/compiletest/src/debuggers.rs +++ b/src/tools/compiletest/src/debuggers.rs @@ -51,6 +51,7 @@ pub(crate) fn configure_gdb(config: &Config) -> Option> { pub(crate) fn configure_lldb(config: &Config) -> Option> { config.lldb_python_dir.as_ref()?; + // FIXME: this is super old if let Some(350) = config.lldb_version { println!( "WARNING: The used version of LLDB (350) has a \ @@ -78,6 +79,7 @@ fn is_pc_windows_msvc_target(target: &str) -> bool { target.ends_with("-pc-windows-msvc") } +/// FIXME: this is very questionable... fn find_cdb(target: &str) -> Option { if !(cfg!(windows) && is_pc_windows_msvc_target(target)) { return None; diff --git a/src/tools/compiletest/src/executor.rs b/src/tools/compiletest/src/executor.rs index 0c4ef36828a0f..df64f12784f5d 100644 --- a/src/tools/compiletest/src/executor.rs +++ b/src/tools/compiletest/src/executor.rs @@ -207,9 +207,9 @@ impl TestOutcome { /// /// Adapted from `filter_tests` in libtest. /// -/// FIXME(#139660): After the libtest dependency is removed, redesign the whole -/// filtering system to do a better job of understanding and filtering _paths_, -/// instead of being tied to libtest's substring/exact matching behaviour. +/// FIXME(#139660): After the libtest dependency is removed, redesign the whole filtering system to +/// do a better job of understanding and filtering _paths_, instead of being tied to libtest's +/// substring/exact matching behaviour. fn filter_tests(opts: &Config, tests: Vec) -> Vec { let mut filtered = tests; @@ -235,9 +235,9 @@ fn filter_tests(opts: &Config, tests: Vec) -> Vec /// /// Copied from `get_concurrency` in libtest. /// -/// FIXME(#139660): After the libtest dependency is removed, consider making -/// bootstrap specify the number of threads on the command-line, instead of -/// propagating the `RUST_TEST_THREADS` environment variable. +/// FIXME(#139660): After the libtest dependency is removed, consider making bootstrap specify the +/// number of threads on the command-line, instead of propagating the `RUST_TEST_THREADS` +/// environment variable. fn get_concurrency() -> usize { if let Ok(value) = env::var("RUST_TEST_THREADS") { match value.parse::>().ok() { diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index dfce4b8b408be..9819079e284f9 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -242,9 +242,12 @@ pub fn parse_config(args: Vec) -> Config { let target = opt_str2(matches.opt_str("target")); let android_cross_path = opt_path(matches, "android-cross-path"); + // FIXME: `cdb_version` is *derived* from cdb, but it's *not* technically a config! let (cdb, cdb_version) = debuggers::analyze_cdb(matches.opt_str("cdb"), &target); + // FIXME: `gdb_version` is *derived* from gdb, but it's *not* technically a config! let (gdb, gdb_version) = debuggers::analyze_gdb(matches.opt_str("gdb"), &target, &android_cross_path); + // FIXME: `lldb_version` is *derived* from lldb, but it's *not* technically a config! let lldb_version = matches.opt_str("lldb-version").as_deref().and_then(debuggers::extract_lldb_version); let color = match matches.opt_str("color").as_deref() { @@ -253,6 +256,9 @@ pub fn parse_config(args: Vec) -> Config { Some("never") => ColorConfig::NeverColor, Some(x) => panic!("argument for --color must be auto, always, or never, but found `{}`", x), }; + // FIXME: this is very questionable, we really should be obtaining LLVM version info from + // `bootstrap`, and not trying to be figuring out that in `compiletest` by running the + // `FileCheck` binary. let llvm_version = matches.opt_str("llvm-version").as_deref().map(directives::extract_llvm_version).or_else( || directives::extract_llvm_version_from_binary(&matches.opt_str("llvm-filecheck")?), @@ -370,6 +376,7 @@ pub fn parse_config(args: Vec) -> Config { mode.parse::() .unwrap_or_else(|_| panic!("unknown `--pass` option `{}` given", mode)) }), + // FIXME: this run scheme is... confusing. run: matches.opt_str("run").and_then(|mode| match mode.as_str() { "auto" => None, "always" => Some(true), @@ -545,6 +552,10 @@ pub fn run_tests(config: Arc) { Some(Debugger::Cdb) => configs.extend(debuggers::configure_cdb(&config)), Some(Debugger::Gdb) => configs.extend(debuggers::configure_gdb(&config)), Some(Debugger::Lldb) => configs.extend(debuggers::configure_lldb(&config)), + // FIXME: the *implicit* debugger discovery makes it really difficult to control + // which {`cdb`, `gdb`, `lldb`} are used. These should **not** be implicitly + // discovered by `compiletest`; these should be explicit `bootstrap` configuration + // options that are passed to `compiletest`! None => { configs.extend(debuggers::configure_cdb(&config)); configs.extend(debuggers::configure_gdb(&config)); diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index f8bf4ee3022e1..0b07bb4da9b40 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -121,6 +121,8 @@ pub fn run(config: Arc, testpaths: &TestPaths, revision: Option<&str>) { } _ => { + // FIXME: this logic seems strange as well. + // android has its own gdb handling if config.debugger == Some(Debugger::Gdb) && config.gdb.is_none() { panic!("gdb not available but debuginfo gdb debuginfo test requested"); @@ -1055,18 +1057,20 @@ impl<'test> TestCx<'test> { let proc_res = match &*self.config.target { // This is pretty similar to below, we're transforming: // - // program arg1 arg2 + // ```text + // program arg1 arg2 + // ``` // // into // - // remote-test-client run program 2 support-lib.so support-lib2.so arg1 arg2 + // ```text + // remote-test-client run program 2 support-lib.so support-lib2.so arg1 arg2 + // ``` // - // The test-client program will upload `program` to the emulator - // along with all other support libraries listed (in this case - // `support-lib.so` and `support-lib2.so`. It will then execute - // the program on the emulator with the arguments specified - // (in the environment we give the process) and then report back - // the same result. + // The test-client program will upload `program` to the emulator along with all other + // support libraries listed (in this case `support-lib.so` and `support-lib2.so`. It + // will then execute the program on the emulator with the arguments specified (in the + // environment we give the process) and then report back the same result. _ if self.config.remote_test_client.is_some() => { let aux_dir = self.aux_output_dir_name(); let ProcArgs { prog, args } = self.make_run_args(); @@ -1532,6 +1536,8 @@ impl<'test> TestCx<'test> { )); // Optionally prevent default --sysroot if specified in test compile-flags. + // + // FIXME: I feel like this logic is fairly sus. if !self.props.compile_flags.iter().any(|flag| flag.starts_with("--sysroot")) && !self.config.host_rustcflags.iter().any(|flag| flag == "--sysroot") { diff --git a/src/tools/compiletest/src/runtest/debuginfo.rs b/src/tools/compiletest/src/runtest/debuginfo.rs index d9e1e4dfc8ddf..471e4a4c81934 100644 --- a/src/tools/compiletest/src/runtest/debuginfo.rs +++ b/src/tools/compiletest/src/runtest/debuginfo.rs @@ -322,6 +322,8 @@ impl TestCx<'_> { &["-quiet".as_ref(), "-batch".as_ref(), "-nx".as_ref(), &debugger_script]; let mut gdb = Command::new(self.config.gdb.as_ref().unwrap()); + + // FIXME: we are propagating `PYTHONPATH` from the environment, not a compiletest flag! let pythonpath = if let Ok(pp) = std::env::var("PYTHONPATH") { format!("{pp}:{rust_pp_module_abs_path}") } else { @@ -443,6 +445,8 @@ impl TestCx<'_> { fn run_lldb(&self, test_executable: &Utf8Path, debugger_script: &Utf8Path) -> ProcRes { // Prepare the lldb_batchmode which executes the debugger script let lldb_script_path = self.config.src_root.join("src/etc/lldb_batchmode.py"); + + // FIXME: `PYTHONPATH` takes precedence over the flag...? let pythonpath = if let Ok(pp) = std::env::var("PYTHONPATH") { format!("{pp}:{}", self.config.lldb_python_dir.as_ref().unwrap()) } else { From 3cd13f72a26b45dd43a24fa8d47f680a9275e046 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 1 Jul 2025 19:21:41 +0200 Subject: [PATCH 3/3] codegen_ssa: replace a Result by an Either --- compiler/rustc_codegen_ssa/src/mir/operand.rs | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/operand.rs b/compiler/rustc_codegen_ssa/src/mir/operand.rs index bf074fec66dd9..2896dfd5463c1 100644 --- a/compiler/rustc_codegen_ssa/src/mir/operand.rs +++ b/compiler/rustc_codegen_ssa/src/mir/operand.rs @@ -1,5 +1,6 @@ use std::fmt; +use itertools::Either; use rustc_abi as abi; use rustc_abi::{ Align, BackendRepr, FIRST_VARIANT, FieldIdx, Primitive, Size, TagEncoding, VariantIdx, Variants, @@ -567,12 +568,12 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { /// Creates an incomplete operand containing the [`abi::Scalar`]s expected based /// on the `layout` passed. This is for use with [`OperandRef::insert_field`] - /// later to set the necessary immediate(s). + /// later to set the necessary immediate(s), one-by-one converting all the `Right` to `Left`. /// /// Returns `None` for `layout`s which cannot be built this way. pub(crate) fn builder( layout: TyAndLayout<'tcx>, - ) -> Option>> { + ) -> Option>> { // Uninhabited types are weird, because for example `Result` // shows up as `FieldsShape::Primitive` and we need to be able to write // a field into `(u32, !)`. We'll do that in an `alloca` instead. @@ -582,15 +583,15 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { let val = match layout.backend_repr { BackendRepr::Memory { .. } if layout.is_zst() => OperandValue::ZeroSized, - BackendRepr::Scalar(s) => OperandValue::Immediate(Err(s)), - BackendRepr::ScalarPair(a, b) => OperandValue::Pair(Err(a), Err(b)), + BackendRepr::Scalar(s) => OperandValue::Immediate(Either::Right(s)), + BackendRepr::ScalarPair(a, b) => OperandValue::Pair(Either::Right(a), Either::Right(b)), BackendRepr::Memory { .. } | BackendRepr::SimdVector { .. } => return None, }; Some(OperandRef { val, layout }) } } -impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result> { +impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Either> { pub(crate) fn insert_field>( &mut self, bx: &mut Bx, @@ -614,29 +615,29 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result> { (field_layout.is_zst(), field_offset == Size::ZERO) }; - let mut update = |tgt: &mut Result, src, from_scalar| { - let to_scalar = tgt.unwrap_err(); + let mut update = |tgt: &mut Either, src, from_scalar| { + let to_scalar = tgt.unwrap_right(); let imm = transmute_scalar(bx, src, from_scalar, to_scalar); - *tgt = Ok(imm); + *tgt = Either::Left(imm); }; match (operand.val, operand.layout.backend_repr) { (OperandValue::ZeroSized, _) if expect_zst => {} (OperandValue::Immediate(v), BackendRepr::Scalar(from_scalar)) => match &mut self.val { - OperandValue::Immediate(val @ Err(_)) if is_zero_offset => { + OperandValue::Immediate(val @ Either::Right(_)) if is_zero_offset => { update(val, v, from_scalar); } - OperandValue::Pair(fst @ Err(_), _) if is_zero_offset => { + OperandValue::Pair(fst @ Either::Right(_), _) if is_zero_offset => { update(fst, v, from_scalar); } - OperandValue::Pair(_, snd @ Err(_)) if !is_zero_offset => { + OperandValue::Pair(_, snd @ Either::Right(_)) if !is_zero_offset => { update(snd, v, from_scalar); } _ => bug!("Tried to insert {operand:?} into {v:?}.{f:?} of {self:?}"), }, (OperandValue::Pair(a, b), BackendRepr::ScalarPair(from_sa, from_sb)) => { match &mut self.val { - OperandValue::Pair(fst @ Err(_), snd @ Err(_)) => { + OperandValue::Pair(fst @ Either::Right(_), snd @ Either::Right(_)) => { update(fst, a, from_sa); update(snd, b, from_sb); } @@ -656,21 +657,21 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result> { let field_offset = self.layout.fields.offset(f.as_usize()); let is_zero_offset = field_offset == Size::ZERO; match &mut self.val { - OperandValue::Immediate(val @ Err(_)) if is_zero_offset => { - *val = Ok(imm); + OperandValue::Immediate(val @ Either::Right(_)) if is_zero_offset => { + *val = Either::Left(imm); } - OperandValue::Pair(fst @ Err(_), _) if is_zero_offset => { - *fst = Ok(imm); + OperandValue::Pair(fst @ Either::Right(_), _) if is_zero_offset => { + *fst = Either::Left(imm); } - OperandValue::Pair(_, snd @ Err(_)) if !is_zero_offset => { - *snd = Ok(imm); + OperandValue::Pair(_, snd @ Either::Right(_)) if !is_zero_offset => { + *snd = Either::Left(imm); } _ => bug!("Tried to insert {imm:?} into field {f:?} of {self:?}"), } } /// After having set all necessary fields, this converts the - /// `OperandValue>` (as obtained from [`OperandRef::builder`]) + /// `OperandValue>` (as obtained from [`OperandRef::builder`]) /// to the normal `OperandValue`. /// /// ICEs if any required fields were not set. @@ -681,13 +682,13 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result> { // payload scalar will not actually have been set, so this converts // unset scalars to corresponding `undef` values so long as the scalar // from the layout allows uninit. - let unwrap = |r: Result| match r { - Ok(v) => v, - Err(s) if s.is_uninit_valid() => { + let unwrap = |r: Either| match r { + Either::Left(v) => v, + Either::Right(s) if s.is_uninit_valid() => { let bty = cx.type_from_scalar(s); cx.const_undef(bty) } - Err(_) => bug!("OperandRef::build called while fields are missing {self:?}"), + Either::Right(_) => bug!("OperandRef::build called while fields are missing {self:?}"), }; let val = match val {