Skip to content

Add if-identical mode for download-ci-llvm #113761

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.

Sign up for GitHub

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
Kobzol wants to merge 3 commits into from
Closed

Add if-identical mode for download-ci-llvm #113761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b53fb47
Store LLVM options in a separate struct
Kobzol Jul 16, 2023
1e6f9a1
Create a `RustDevConfig` step that stores the LLVM options used to co…
Kobzol Jul 16, 2023
de8e02c
Add a new `if-identical` mode to `llvm.from_ci`
Kobzol Jul 16, 2023
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
4 changes: 2 additions & 2 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ impl<'a> Builder<'a> {
let mut dylib_dirs = vec![self.rustc_libdir(compiler)];

// Ensure that the downloaded LLVM libraries can be found.
if self.config.llvm_from_ci {
if self.config.llvm.from_ci {
let ci_llvm_lib = self.out.join(&*compiler.host.triple).join("ci-llvm").join("lib");
dylib_dirs.push(ci_llvm_lib);
}
Expand Down Expand Up @@ -1872,7 +1872,7 @@ impl<'a> Builder<'a> {
//
// FIXME: the guard against msvc shouldn't need to be here
if target.contains("msvc") {
if let Some(ref cl) = self.config.llvm_clang_cl {
if let Some(ref cl) = self.config.llvm.clang_cl {
cargo.env("CC", cl).env("CXX", cl);
}
} else {
Expand Down
14 changes: 7 additions & 7 deletions src/bootstrap/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ impl Step for Rustc {
// is already on by default in MSVC optimized builds, which is interpreted as --icf=all:
// https://github.com/llvm/llvm-project/blob/3329cec2f79185bafd678f310fafadba2a8c76d2/lld/COFF/Driver.cpp#L1746
// https://github.com/rust-lang/rust/blob/f22819bcce4abaff7d1246a56eec493418f9f4ee/compiler/rustc_codegen_ssa/src/back/linker.rs#L827
if builder.config.use_lld && !compiler.host.contains("msvc") {
if builder.config.llvm.use_lld && !compiler.host.contains("msvc") {
cargo.rustflag("-Clink-args=-Wl,--icf=all");
}

Expand Down Expand Up @@ -1002,15 +1002,15 @@ fn rustc_llvm_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetSelect
// `__llvm_profile_instrument_memop` when linking `rustc_driver`.
let mut llvm_linker_flags = String::new();
if builder.config.llvm_profile_generate && target.contains("msvc") {
if let Some(ref clang_cl_path) = builder.config.llvm_clang_cl {
if let Some(ref clang_cl_path) = builder.config.llvm.clang_cl {
// Add clang's runtime library directory to the search path
let clang_rt_dir = get_clang_cl_resource_dir(clang_cl_path);
llvm_linker_flags.push_str(&format!("-L{}", clang_rt_dir.display()));
}
}

// The config can also specify its own llvm linker flags.
if let Some(ref s) = builder.config.llvm_ldflags {
if let Some(ref s) = builder.config.llvm.ldflags {
if !llvm_linker_flags.is_empty() {
llvm_linker_flags.push_str(" ");
}
Expand All @@ -1024,7 +1024,7 @@ fn rustc_llvm_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetSelect

// Building with a static libstdc++ is only supported on linux right now,
// not for MSVC or macOS
if builder.config.llvm_static_stdcpp
if builder.config.llvm.static_stdcpp
&& !target.contains("freebsd")
&& !target.contains("msvc")
&& !target.contains("apple")
Expand All @@ -1042,10 +1042,10 @@ fn rustc_llvm_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetSelect
if builder.llvm_link_shared() {
cargo.env("LLVM_LINK_SHARED", "1");
}
if builder.config.llvm_use_libcxx {
if builder.config.llvm.use_libcxx {
cargo.env("LLVM_USE_LIBCXX", "1");
}
if builder.config.llvm_optimize && !builder.config.llvm_release_debuginfo {
if builder.config.llvm.optimize && !builder.config.llvm.release_debuginfo {
cargo.env("LLVM_NDEBUG", "1");
}
}
Expand Down Expand Up @@ -1564,7 +1564,7 @@ impl Step for Assemble {
});
}

let lld_install = if builder.config.lld_enabled {
let lld_install = if builder.config.llvm.lld_enabled {
Some(builder.ensure(llvm::Lld { target: target_compiler.host }))
} else {
None
Expand Down
149 changes: 77 additions & 72 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,43 @@ impl Display for DebuginfoLevel {
}
}

#[derive(Default, Clone)]
pub struct LLVMConfig {
Copy link
Member

@jyn514 jyn514 Jul 16, 2023

Choose a reason for hiding this comment

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

this is essentially the same as config::Llvm struct, right? can we reuse that instead of adding a new struct?

(btw ty for putting this in a separate commit, it made things a lot easier to review ❤️)

Copy link
Member Author

@Kobzol Kobzol Jul 17, 2023

Choose a reason for hiding this comment

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

The goal of this struct is to have a "ground truth" representation of all the options that can affect the build of LLVM, so that we can compare the CI version and the local version in an easy way - by just comparing the value of a struct, without manually comparing a subset of its keys.

However, there are some options under the [llvm] key in config.toml which should not be compared - notably download-ci-llvm. We definitely do not want to include this key when comparing the CI and local configs. So this is not 1:1 with the LLVM struct.

Note: I only removed from_ci from this struct in a later commit, to uphold what I wrote above, which is probably confusing.

pub assertions: bool,
pub tests: bool,
pub plugins: bool,
pub optimize: bool,
pub thin_lto: bool,
pub release_debuginfo: bool,
pub static_stdcpp: bool,
/// `None` if `llvm_from_ci` is true and we haven't yet downloaded llvm.
#[cfg(not(test))]
link_shared: Cell<Option<bool>>,
#[cfg(test)]
pub link_shared: Cell<Option<bool>>,
pub clang_cl: Option<String>,
pub targets: Option<String>,
pub experimental_targets: Option<String>,
pub link_jobs: Option<u32>,
pub version_suffix: Option<String>,
pub use_linker: Option<String>,
pub allow_old_toolchain: bool,
pub polly: bool,
pub clang: bool,
pub enable_warnings: bool,
pub from_ci: bool,
pub build_config: HashMap<String, String>,

pub use_lld: bool,
pub lld_enabled: bool,
pub tools_enabled: bool,

pub cflags: Option<String>,
pub cxxflags: Option<String>,
pub ldflags: Option<String>,
pub use_libcxx: bool,
}

/// Global configuration for the entire build and/or bootstrap.
///
/// This structure is parsed from `config.toml`, and some of the fields are inferred from `git` or build-time parameters.
Expand Down Expand Up @@ -167,39 +204,7 @@ pub struct Config {
pub backtrace_on_ice: bool,

// llvm codegen options
pub llvm_assertions: bool,
pub llvm_tests: bool,
pub llvm_plugins: bool,
pub llvm_optimize: bool,
pub llvm_thin_lto: bool,
pub llvm_release_debuginfo: bool,
pub llvm_static_stdcpp: bool,
/// `None` if `llvm_from_ci` is true and we haven't yet downloaded llvm.
#[cfg(not(test))]
llvm_link_shared: Cell<Option<bool>>,
#[cfg(test)]
pub llvm_link_shared: Cell<Option<bool>>,
pub llvm_clang_cl: Option<String>,
pub llvm_targets: Option<String>,
pub llvm_experimental_targets: Option<String>,
pub llvm_link_jobs: Option<u32>,
pub llvm_version_suffix: Option<String>,
pub llvm_use_linker: Option<String>,
pub llvm_allow_old_toolchain: bool,
pub llvm_polly: bool,
pub llvm_clang: bool,
pub llvm_enable_warnings: bool,
pub llvm_from_ci: bool,
pub llvm_build_config: HashMap<String, String>,

pub use_lld: bool,
pub lld_enabled: bool,
pub llvm_tools_enabled: bool,

pub llvm_cflags: Option<String>,
pub llvm_cxxflags: Option<String>,
pub llvm_ldflags: Option<String>,
pub llvm_use_libcxx: bool,
pub llvm: LLVMConfig,

// rust codegen options
pub rust_optimize: RustOptimize,
Expand Down Expand Up @@ -1052,9 +1057,9 @@ define_config! {
impl Config {
pub fn default_opts() -> Config {
let mut config = Config::default();
config.llvm_optimize = true;
config.llvm.optimize = true;
config.ninja_in_file = true;
config.llvm_static_stdcpp = false;
config.llvm.static_stdcpp = false;
config.backtrace = true;
config.rust_optimize = RustOptimize::Bool(true);
config.rust_optimize_tests = true;
Expand Down Expand Up @@ -1428,9 +1433,9 @@ impl Config {
if let Some(true) = rust.incremental {
config.incremental = true;
}
set(&mut config.use_lld, rust.use_lld);
set(&mut config.lld_enabled, rust.lld);
set(&mut config.llvm_tools_enabled, rust.llvm_tools);
set(&mut config.llvm.use_lld, rust.use_lld);
set(&mut config.llvm.lld_enabled, rust.lld);
set(&mut config.llvm.tools_enabled, rust.llvm_tools);
config.rustc_parallel = rust.parallel_compiler.unwrap_or(false);
config.rustc_default_linker = rust.default_linker;
config.musl_root = rust.musl_root.map(PathBuf::from);
Expand Down Expand Up @@ -1489,32 +1494,32 @@ impl Config {
llvm_assertions = llvm.assertions;
llvm_tests = llvm.tests;
llvm_plugins = llvm.plugins;
set(&mut config.llvm_optimize, llvm.optimize);
set(&mut config.llvm_thin_lto, llvm.thin_lto);
set(&mut config.llvm_release_debuginfo, llvm.release_debuginfo);
set(&mut config.llvm_static_stdcpp, llvm.static_libstdcpp);
set(&mut config.llvm.optimize, llvm.optimize);
set(&mut config.llvm.thin_lto, llvm.thin_lto);
set(&mut config.llvm.release_debuginfo, llvm.release_debuginfo);
set(&mut config.llvm.static_stdcpp, llvm.static_libstdcpp);
if let Some(v) = llvm.link_shared {
config.llvm_link_shared.set(Some(v));
config.llvm.link_shared.set(Some(v));
}
config.llvm_targets = llvm.targets.clone();
config.llvm_experimental_targets = llvm.experimental_targets.clone();
config.llvm_link_jobs = llvm.link_jobs;
config.llvm_version_suffix = llvm.version_suffix.clone();
config.llvm_clang_cl = llvm.clang_cl.clone();

config.llvm_cflags = llvm.cflags.clone();
config.llvm_cxxflags = llvm.cxxflags.clone();
config.llvm_ldflags = llvm.ldflags.clone();
set(&mut config.llvm_use_libcxx, llvm.use_libcxx);
config.llvm_use_linker = llvm.use_linker.clone();
config.llvm_allow_old_toolchain = llvm.allow_old_toolchain.unwrap_or(false);
config.llvm_polly = llvm.polly.unwrap_or(false);
config.llvm_clang = llvm.clang.unwrap_or(false);
config.llvm_enable_warnings = llvm.enable_warnings.unwrap_or(false);
config.llvm_build_config = llvm.build_config.clone().unwrap_or(Default::default());
config.llvm.targets = llvm.targets.clone();
config.llvm.experimental_targets = llvm.experimental_targets.clone();
config.llvm.link_jobs = llvm.link_jobs;
config.llvm.version_suffix = llvm.version_suffix.clone();
config.llvm.clang_cl = llvm.clang_cl.clone();

config.llvm.cflags = llvm.cflags.clone();
config.llvm.cxxflags = llvm.cxxflags.clone();
config.llvm.ldflags = llvm.ldflags.clone();
set(&mut config.llvm.use_libcxx, llvm.use_libcxx);
config.llvm.use_linker = llvm.use_linker.clone();
config.llvm.allow_old_toolchain = llvm.allow_old_toolchain.unwrap_or(false);
config.llvm.polly = llvm.polly.unwrap_or(false);
config.llvm.clang = llvm.clang.unwrap_or(false);
config.llvm.enable_warnings = llvm.enable_warnings.unwrap_or(false);
config.llvm.build_config = llvm.build_config.clone().unwrap_or(Default::default());

let asserts = llvm_assertions.unwrap_or(false);
config.llvm_from_ci = match llvm.download_ci_llvm {
config.llvm.from_ci = match llvm.download_ci_llvm {
Some(StringOrBool::String(s)) => {
assert!(s == "if-available", "unknown option `{}` for download-ci-llvm", s);
crate::llvm::is_ci_llvm_available(&config, asserts)
Expand All @@ -1525,7 +1530,7 @@ impl Config {
}
};

if config.llvm_from_ci {
if config.llvm.from_ci {
// None of the LLVM options, except assertions, are supported
// when using downloaded LLVM. We could just ignore these but
// that's potentially confusing, so force them to not be
Expand Down Expand Up @@ -1556,14 +1561,14 @@ impl Config {
}

// NOTE: can never be hit when downloading from CI, since we call `check_ci_llvm!(thin_lto)` above.
if config.llvm_thin_lto && llvm.link_shared.is_none() {
if config.llvm.thin_lto && llvm.link_shared.is_none() {
// If we're building with ThinLTO on, by default we want to link
// to LLVM shared, to avoid re-doing ThinLTO (which happens in
// the link step) with each stage.
config.llvm_link_shared.set(Some(true));
config.llvm.link_shared.set(Some(true));
}
} else {
config.llvm_from_ci =
config.llvm.from_ci =
config.channel == "dev" && crate::llvm::is_ci_llvm_available(&config, false);
}

Expand Down Expand Up @@ -1615,7 +1620,7 @@ impl Config {
}
}

if config.llvm_from_ci {
if config.llvm.from_ci {
let triple = &config.build.triple;
let ci_llvm_bin = config.ci_llvm_root().join("bin");
let build_target = config
Expand Down Expand Up @@ -1650,9 +1655,9 @@ impl Config {
// Now that we've reached the end of our configuration, infer the
// default values for all options that we haven't otherwise stored yet.

config.llvm_assertions = llvm_assertions.unwrap_or(false);
config.llvm_tests = llvm_tests.unwrap_or(false);
config.llvm_plugins = llvm_plugins.unwrap_or(false);
config.llvm.assertions = llvm_assertions.unwrap_or(false);
config.llvm.tests = llvm_tests.unwrap_or(false);
config.llvm.plugins = llvm_plugins.unwrap_or(false);
config.rust_optimize = optimize.unwrap_or(RustOptimize::Bool(true));

let default = debug == Some(true);
Expand Down Expand Up @@ -1855,7 +1860,7 @@ impl Config {

/// The absolute path to the downloaded LLVM artifacts.
pub(crate) fn ci_llvm_root(&self) -> PathBuf {
assert!(self.llvm_from_ci);
assert!(self.llvm.from_ci);
self.out.join(&*self.build.triple).join("ci-llvm")
}

Expand All @@ -1870,14 +1875,14 @@ impl Config {
/// If `false`, llvm should be linked statically.
/// This is computed on demand since LLVM might have to first be downloaded from CI.
pub(crate) fn llvm_link_shared(&self) -> bool {
let mut opt = self.llvm_link_shared.get();
let mut opt = self.llvm.link_shared.get();
if opt.is_none() && self.dry_run() {
// just assume static for now - dynamic linking isn't supported on all platforms
return false;
}

let llvm_link_shared = *opt.get_or_insert_with(|| {
if self.llvm_from_ci {
if self.llvm.from_ci {
self.maybe_download_ci_llvm();
let ci_llvm = self.ci_llvm_root();
let link_type = t!(
Expand All @@ -1891,7 +1896,7 @@ impl Config {
false
}
});
self.llvm_link_shared.set(opt);
self.llvm.link_shared.set(opt);
llvm_link_shared
}

Expand Down
8 changes: 4 additions & 4 deletions src/bootstrap/dist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ impl Step for Rustc {
t!(fs::create_dir_all(&dst_dir));

// Copy over lld if it's there
if builder.config.lld_enabled {
if builder.config.llvm.lld_enabled {
let src_dir = builder.sysroot_libdir(compiler, host).parent().unwrap().join("bin");
let rust_lld = exe("rust-lld", compiler.host);
builder.copy(&src_dir.join(&rust_lld), &dst_dir.join(&rust_lld));
Expand Down Expand Up @@ -1968,7 +1968,7 @@ fn install_llvm_file(builder: &Builder<'_>, source: &Path, destination: &Path) {
/// Returns whether the files were actually copied.
fn maybe_install_llvm(builder: &Builder<'_>, target: TargetSelection, dst_libdir: &Path) -> bool {
if let Some(config) = builder.config.target_config.get(&target) {
if config.llvm_config.is_some() && !builder.config.llvm_from_ci {
if config.llvm_config.is_some() && !builder.config.llvm.from_ci {
// If the LLVM was externally provided, then we don't currently copy
// artifacts into the sysroot. This is not necessarily the right
// choice (in particular, it will require the LLVM dylib to be in
Expand Down Expand Up @@ -2090,7 +2090,7 @@ impl Step for BoltInstrument {
return self.file.clone();
}

if builder.build.config.llvm_from_ci {
if builder.build.config.llvm.from_ci {
println!("warning: trying to use BOLT with LLVM from CI, this will probably not work");
}

Expand Down Expand Up @@ -2142,7 +2142,7 @@ impl Step for BoltOptimize {
return self.file.clone();
}

if builder.build.config.llvm_from_ci {
if builder.build.config.llvm.from_ci {
println!("warning: trying to use BOLT with LLVM from CI, this will probably not work");
}

Expand Down
Loading