Skip to content

Allow running tests with GCC #144687

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
10 changes: 9 additions & 1 deletion src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1769,7 +1769,10 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg("--host").arg(&*compiler.host.triple);
cmd.arg("--llvm-filecheck").arg(builder.llvm_filecheck(builder.config.host_target));

if let Some(codegen_backend) = builder.config.default_codegen_backend(compiler.host) {
if let Some(codegen_backend) = builder.config.codegen_backends(compiler.host).first() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't seem to run commands for each registered backend, I simply look at the first for now. Should I add a FIXME comment on it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change, why didn't you just use default_codegen_backend? It does the same thing (looks at the first codegen backend).

Copy link
Member

@bjorn3 bjorn3 Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_codegen_backend() is literally implemented as codegen_backends().first().
Edit: for some reason github didn't show me Kobzol's comment before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just submitted it now 😆 It was a pending comment, but GH uses the original time when it was written, not when it was submitted, which is a bit confusing.

cmd.arg("--codegen-backend").arg(codegen_backend);
} else if let Some(codegen_backend) = builder.config.default_codegen_backend(compiler.host)
{
cmd.arg("--codegen-backend").arg(&codegen_backend);
}

Expand Down Expand Up @@ -1917,6 +1920,11 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg(exclude);
}

if builder.config.codegen_backends(target).first().map(|b| b.as_str()) == Some("gcc") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

cmd.arg("--extra-library-path");
cmd.arg(builder.config.libgccjit_root());
}

// Get paths from cmd args
let paths = match &builder.config.cmd {
Subcommand::Test { .. } => &builder.config.paths[..],
Expand Down
6 changes: 6 additions & 0 deletions src/bootstrap/src/core/builder/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,12 @@ impl Builder<'_> {
println!("using sysroot {sysroot_str}");
}

if self.config.codegen_backends(target).first().map(|b| b.as_str()) == Some("gcc") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

let gcc_sysroot = self.config.libgccjit_root();
let gcc_sysroot = gcc_sysroot.as_os_str().to_str().expect("sysroot should be UTF-8");
cargo.env("LD_LIBRARY_PATH", gcc_sysroot);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it done for cg_llvm?
Couldn't we reuse what is done there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't libgccjit copied into the sysroot like for libLLVM?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent question, absolutely no clue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for build, but not for dist, we do not want to dist libgccjit.so by default with rustup (at least not yet). If build did this automatically, we would have to explicitly skip it when creating the dist archives (normally we mostly copy all .so files to them, I think).

Copy link
Member

@bjorn3 bjorn3 Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dist has an explicit list of files to include in each component, right? It has to for each .so files to not end up in every component. So the cg_gcc component can include just cg_gcc.so and not libgccjit.so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's very explicit in this case:

let libdir = builder.rustc_libdir(compiler);

To clarify, the cg_gcc rustup component should include both cg_gcc.so and libgccjit.so. But the normal rustc component shouldn't contain libgccjit.so.

}

let mut rustflags = Rustflags::new(target);
if build_compiler_stage != 0 {
if let Ok(s) = env::var("CARGOFLAGS_NOT_BOOTSTRAP") {
Expand Down
7 changes: 7 additions & 0 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,13 @@ impl Config {
self.out.join(self.host_target).join("ci-rustc")
}

pub(crate) fn libgccjit_root(&self) -> PathBuf {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we don't store GCC in the same ___location depending whether we build it or we download it, I found it better to handle it in one place instead, hence the creation of this method.

match self.gcc_ci_mode {
GccCiMode::BuildLocally => self.out.join(self.host_target).join("gcc/install/lib"),
GccCiMode::DownloadFromCi => self.out.join(self.host_target).join("ci-gcc/lib"),
}
}

/// Determine whether llvm should be linked dynamically.
///
/// If `false`, llvm should be linked statically.
Expand Down
10 changes: 9 additions & 1 deletion src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,9 @@ pub struct Config {

/// Current codegen backend used.
pub codegen_backend: CodegenBackend,

/// Only used by GCC codegen backend for now. It is used to pass `libgccjit.so`'s path.
pub extra_library_path: Option<Utf8PathBuf>,
}

impl Config {
Expand Down Expand Up @@ -787,6 +790,7 @@ impl Config {
diff_command: Default::default(),
minicore_path: Default::default(),
codegen_backend: CodegenBackend::Llvm,
extra_library_path: None,
}
}

Expand Down Expand Up @@ -1151,7 +1155,11 @@ fn supported_crate_types(config: &Config) -> HashSet<String> {

fn rustc_output(config: &Config, args: &[&str], envs: HashMap<String, String>) -> String {
let mut command = Command::new(&config.rustc_path);
add_dylib_path(&mut command, iter::once(&config.compile_lib_path));
if let Some(ref extra_lib) = config.extra_library_path {
add_dylib_path(&mut command, [&config.compile_lib_path, &extra_lib].iter());
} else {
add_dylib_path(&mut command, iter::once(&config.compile_lib_path));
}
command.args(&config.target_rustcflags).args(args);
command.env("RUSTC_BOOTSTRAP", "1");
command.envs(envs);
Expand Down
11 changes: 8 additions & 3 deletions src/tools/compiletest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,12 @@ pub fn parse_config(args: Vec<String>) -> Config {
"only test a specific debugger in debuginfo tests",
"gdb | lldb | cdb",
)
.optopt("", "codegen-backend", "the codegen backend currently used", "CODEGEN BACKEND NAME")
.optopt(
"",
"codegen-backend",
"the codegen backend currently used",
"CODEGEN BACKEND NAME",
"extra-library-path",
"extra path to be passed into LIBRARY_PATH environment variable",
"library path",
);

let (argv0, args_) = args.split_first().unwrap();
Expand Down Expand Up @@ -466,6 +467,10 @@ pub fn parse_config(args: Vec<String>) -> Config {
minicore_path: opt_path(matches, "minicore-path"),

codegen_backend,

extra_library_path: matches
.opt_str("extra-library-path")
.map(|p| make_absolute(Utf8PathBuf::from(p))),
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1556,6 +1556,11 @@ impl<'test> TestCx<'test> {
// In stage 0, make sure we use `stage0-sysroot` instead of the bootstrap sysroot.
rustc.arg("--sysroot").arg(&self.config.sysroot_base);
}
// If the provided codegen backend is not LLVM, we need to pass it.
if !matches!(self.config.codegen_backend, crate::CodegenBackend::Llvm) {
let lib_path = self.config.sysroot_base.join(self.config.codegen_backend.as_str());
rustc.arg(format!("-Zcodegen-backend={}", lib_path));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always matches the default codegen backend, right? In that case no explicit -Zcodegen-backend should be necessary.


// Optionally prevent default --target if specified in test compile-flags.
let custom_target = self.props.compile_flags.iter().any(|x| x.starts_with("--target"));
Expand Down
Loading