Skip to content
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

Build library/profiler_builtins from ci-llvm if appropriate #129295

Merged
merged 2 commits into from
Aug 25, 2024
Merged
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
16 changes: 12 additions & 4 deletions library/profiler_builtins/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! See the build.rs for libcompiler_builtins crate for details.

use std::env;
use std::path::Path;
use std::path::PathBuf;

fn main() {
println!("cargo:rerun-if-env-changed=LLVM_PROFILER_RT_LIB");
Expand Down Expand Up @@ -79,17 +79,25 @@ fn main() {
cfg.define("COMPILER_RT_HAS_ATOMICS", Some("1"));
}

// Note that this should exist if we're going to run (otherwise we just
// don't build profiler builtins at all).
let root = Path::new("../../src/llvm-project/compiler-rt");
// Get the LLVM `compiler-rt` directory from bootstrap.
println!("cargo:rerun-if-env-changed=RUST_COMPILER_RT_FOR_PROFILER");
let root = PathBuf::from(env::var("RUST_COMPILER_RT_FOR_PROFILER").unwrap_or_else(|_| {
let path = "../../src/llvm-project/compiler-rt";
println!("RUST_COMPILER_RT_FOR_PROFILER was not set; falling back to {path:?}");
path.to_owned()
}));

let src_root = root.join("lib").join("profile");
assert!(src_root.exists(), "profiler runtime source directory not found: {src_root:?}");
let mut n_sources_found = 0u32;
for src in profile_sources {
let path = src_root.join(src);
if path.exists() {
cfg.file(path);
n_sources_found += 1;
}
}
assert!(n_sources_found > 0, "couldn't find any profiler runtime source files in {src_root:?}");

cfg.include(root.join("include"));
cfg.warnings(false);
Expand Down
44 changes: 32 additions & 12 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,16 +199,6 @@ impl Step for Std {

builder.require_submodule("library/stdarch", None);

// Profiler information requires LLVM's compiler-rt
if builder.config.profiler {
builder.require_submodule(
"src/llvm-project",
Some(
"The `build.profiler` config option requires `compiler-rt` sources from LLVM.",
),
);
}

let mut target_deps = builder.ensure(StartupObjects { compiler, target });

let compiler_to_use = builder.compiler_for(compiler.stage, compiler.host, target);
Expand Down Expand Up @@ -466,15 +456,45 @@ pub fn std_crates_for_run_make(run: &RunConfig<'_>) -> Vec<String> {
}
}

/// Tries to find LLVM's `compiler-rt` source directory, for building `library/profiler_builtins`.
///
/// Normally it lives in the `src/llvm-project` submodule, but if we will be using a
/// downloaded copy of CI LLVM, then we try to use the `compiler-rt` sources from
/// there instead, which lets us avoid checking out the LLVM submodule.
fn compiler_rt_for_profiler(builder: &Builder<'_>) -> PathBuf {
// Try to use `compiler-rt` sources from downloaded CI LLVM, if possible.
if builder.config.llvm_from_ci {
// CI LLVM might not have been downloaded yet, so try to download it now.
builder.config.maybe_download_ci_llvm();
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit worried about this running in the dry run mode, but it seems like LLVM runs maybe_download_ci_llvm in dry run too, so it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like maybe_download_ci_llvm has a !self.dry_run() guard before it actually downloads anything, so the actual download should be skipped.

(This does mean that dry-run and normal invocations might disagree on what path will be used for compiler-rt.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I put a log inside the maybe_download function and it was also printed during the dry run (even without your PR). But maybe I missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the check I have in mind:

if program_out_of_date(&llvm_stamp, &key) && !self.dry_run() {

Notice that it’s a bit further down from the start of the function, so perhaps your log message was before it.

let ci_llvm_compiler_rt = builder.config.ci_llvm_root().join("compiler-rt");
if ci_llvm_compiler_rt.exists() {
return ci_llvm_compiler_rt;
}
}

// Otherwise, fall back to requiring the LLVM submodule.
builder.require_submodule("src/llvm-project", {
Some("The `build.profiler` config option requires `compiler-rt` sources from LLVM.")
});
builder.src.join("src/llvm-project/compiler-rt")
}

/// Configure cargo to compile the standard library, adding appropriate env vars
/// and such.
pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, cargo: &mut Cargo) {
if let Some(target) = env::var_os("MACOSX_STD_DEPLOYMENT_TARGET") {
cargo.env("MACOSX_DEPLOYMENT_TARGET", target);
}

// Paths needed by `library/profiler_builtins/build.rs`.
if let Some(path) = builder.config.profiler_path(target) {
cargo.env("LLVM_PROFILER_RT_LIB", path);
} else if builder.config.profiler_enabled(target) {
let compiler_rt = compiler_rt_for_profiler(builder);
// Currently this is separate from the env var used by `compiler_builtins`
// (below) so that adding support for CI LLVM here doesn't risk breaking
// the compiler builtins. But they could be unified if desired.
cargo.env("RUST_COMPILER_RT_FOR_PROFILER", compiler_rt);
}

// Determine if we're going to compile in optimized C intrinsics to
Expand Down Expand Up @@ -507,8 +527,8 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
);
let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
assert!(compiler_builtins_root.exists());
// Note that `libprofiler_builtins/build.rs` also computes this so if
// you're changing something here please also change that.
// The path to `compiler-rt` is also used by `profiler_builtins` (above),
// so if you're changing something here please also change that as appropriate.
cargo.env("RUST_COMPILER_RT_ROOT", &compiler_builtins_root);
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
" compiler-builtins-c"
} else {
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/utils/change_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
severity: ChangeSeverity::Warning,
summary: "./x test --rustc-args was renamed to --compiletest-rustc-args as it only applies there. ./x miri --rustc-args was also removed.",
},
ChangeInfo {
change_id: 129295,
severity: ChangeSeverity::Info,
summary: "The `build.profiler` option now tries to use source code from `download-ci-llvm` if possible, instead of checking out the `src/llvm-project` submodule.",
},
];
Loading