Skip to content

Enable debug assertions on alt builds #131077

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ci/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ if [ "$DEPLOY$DEPLOY_ALT" = "1" ]; then

CODEGEN_BACKENDS="${CODEGEN_BACKENDS:-llvm}"
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.codegen-backends=$CODEGEN_BACKENDS"

# Unless explicitly disabled, we want rustc debug assertions on the -alt builds
if [ "$DEPLOY_ALT" != "" ] && [ "$NO_DEBUG_ASSERTIONS" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-debug-assertions"
fi
else
# We almost always want debug assertions enabled, but sometimes this takes too
# long for too little benefit, so we just turn them off.
Expand Down
36 changes: 36 additions & 0 deletions src/tools/opt-dist/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,27 @@ pub struct Environment {
prebuilt_rustc_perf: Option<Utf8PathBuf>,
use_bolt: bool,
shared_llvm: bool,
/// Additional configuration that bootstrap needs to know only when running tests.
#[builder(default)]
test_config: TestConfig,
}

/// Builds have optional components, and their presence/absence can enable/disable a subset of
/// tests. When testing the optimized artifacts, bootstrap needs to know about these enabled
/// components to run the expected subset. This structure holds the known components where this
/// matters: currently only whether the build to test is using debug assertions.
///
/// FIXME: ultimately, this is a temporary band-aid, and opt-dist should be more transparent to the
/// CI config and bootstrap optional components: bootstrap has default values, combinations of flags
/// that cascade into others, etc logic that we'd have to duplicate here otherwise. It's more
/// sensible for opt-dist to never know about the config apart from the minimal set of paths
/// required to configure stage0 tests.
#[derive(Builder, Default, Clone, Debug)]
pub struct TestConfig {
/// Whether the build under test is explicitly using `--enable-debug-assertions`.
/// Note that this flag can be implied from others, like `rust.debug`, and we do not handle any
/// of these subtleties and defaults here, as per the FIXME above.
pub rust_debug_assertions: bool,
}

impl Environment {
Expand Down Expand Up @@ -101,6 +122,21 @@ impl Environment {
pub fn benchmark_cargo_config(&self) -> &[String] {
&self.benchmark_cargo_config
}

pub fn test_config(&self) -> &TestConfig {
&self.test_config
}
}

impl TestConfig {
/// Returns the test config matching the given `RUST_CONFIGURE_ARGS` for the known optional
/// components for tests. This is obviously extremely fragile and we'd rather opt-dist not
/// handle any optional components.
pub fn from_configure_args(configure_args: &str) -> TestConfig {
let enable_debug_assertions =
configure_args.split(" ").find(|part| *part == "--enable-debug-assertions").is_some();
TestConfig { rust_debug_assertions: enable_debug_assertions }
}
}

/// What is the extension of binary executables on this platform?
Expand Down
13 changes: 13 additions & 0 deletions src/tools/opt-dist/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use anyhow::Context;
use camino::{Utf8Path, Utf8PathBuf};
use clap::Parser;
use environment::TestConfig;
use log::LevelFilter;
use utils::io;

Expand Down Expand Up @@ -148,6 +149,11 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)>

let is_aarch64 = target_triple.starts_with("aarch64");

// Parse the optional build components that impact the test environment.
let rust_configure_args = std::env::var("RUST_CONFIGURE_ARGS")
.expect("RUST_CONFIGURE_ARGS environment variable missing");
let test_config = TestConfig::from_configure_args(&rust_configure_args);

let checkout_dir = Utf8PathBuf::from("/checkout");
let env = EnvironmentBuilder::default()
.host_tuple(target_triple)
Expand All @@ -160,6 +166,7 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)>
// FIXME: Enable bolt for aarch64 once it's fixed upstream. Broken as of December 2024.
.use_bolt(!is_aarch64)
.skipped_tests(vec![])
.test_config(test_config)
.build()?;

(env, shared.build_args)
Expand All @@ -168,6 +175,11 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)>
let target_triple =
std::env::var("PGO_HOST").expect("PGO_HOST environment variable missing");

// Parse the optional build components that impact the test environment.
let rust_configure_args = std::env::var("RUST_CONFIGURE_ARGS")
.expect("RUST_CONFIGURE_ARGS environment variable missing");
let test_config = TestConfig::from_configure_args(&rust_configure_args);

let checkout_dir: Utf8PathBuf = std::env::current_dir()?.try_into()?;
let env = EnvironmentBuilder::default()
.host_tuple(target_triple)
Expand All @@ -179,6 +191,7 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)>
.shared_llvm(false)
.use_bolt(false)
.skipped_tests(vec![])
.test_config(test_config)
.build()?;

(env, shared.build_args)
Expand Down
4 changes: 3 additions & 1 deletion src/tools/opt-dist/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ change-id = 115898
[rust]
channel = "{channel}"
verbose-tests = true
debug-assertions = {debug_assertions}

[build]
rustc = "{rustc}"
Expand All @@ -83,7 +84,8 @@ llvm-config = "{llvm_config}"
"#,
rustc = rustc_path.to_string().replace('\\', "/"),
cargo = cargo_path.to_string().replace('\\', "/"),
llvm_config = llvm_config.to_string().replace('\\', "/")
llvm_config = llvm_config.to_string().replace('\\', "/"),
debug_assertions = env.test_config().rust_debug_assertions,
);
log::info!("Using following `bootstrap.toml` for running tests:\n{config_content}");

Expand Down
Loading