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

Make the c feature for compiler-builtins an explicit opt-in #101833

Merged
merged 1 commit into from
Sep 29, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions config.toml.example
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ changelog-seen = 2
# on this runtime, such as `-C profile-generate` or `-C instrument-coverage`).
#profiler = false

# Use the optimized LLVM C intrinsics for `compiler_builtins`, rather than Rust intrinsics.
# Requires the LLVM submodule to be managed by bootstrap (i.e. not external).
#optimized-compiler-builtins = false

# Indicates whether the native libraries linked into Cargo will be statically
# linked or not.
#cargo-native-static = false
Expand Down
15 changes: 10 additions & 5 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,7 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car

// Determine if we're going to compile in optimized C intrinsics to
// the `compiler-builtins` crate. These intrinsics live in LLVM's
// `compiler-rt` repository, but our `src/llvm-project` submodule isn't
// always checked out, so we need to conditionally look for this. (e.g. if
// an external LLVM is used we skip the LLVM submodule checkout).
// `compiler-rt` repository.
//
// Note that this shouldn't affect the correctness of `compiler-builtins`,
// but only its speed. Some intrinsics in C haven't been translated to Rust
Expand All @@ -312,8 +310,15 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
// If `compiler-rt` is available ensure that the `c` feature of the
// `compiler-builtins` crate is enabled and it's configured to learn where
// `compiler-rt` is located.
let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
let compiler_builtins_c_feature = if compiler_builtins_root.exists() {
let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins {
if !builder.is_rust_llvm(target) {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this should be checking if a source checkout of LLVM is being used. It shouldn't be affected by the llvm-has-rust-patches target option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think is_rust_llvm is exactly what we want here, actually. We want to know that if we use the sources in src/llvm-project, the instrumentation there will match the instrumentation in libLLVM.so, which is true exactly when that has the same patches as the submodule. Whether it was originally built from source or not shouldn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you want to know if the versions match, which isn't the same as whether you're building from source.. it also isn't the same as whether it includes Rust patches, though. I commented on the other PR

panic!(
"need a managed LLVM submodule for optimized intrinsics support; unset `llvm-config` or `optimized-compiler-builtins`"
);
Comment on lines +315 to +317
Copy link
Member

Choose a reason for hiding this comment

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

If this check is present, there has to be a way to provide the path to the compiler-rt source code of the custom LLVM, otherwise it will be impossible to use custom LLVMs on some platforms (see #101833 (comment)).

What I would do is have a check like:

if optimized_compiler_builtins && (!is_rust_llvm() || compiler_rt_src_path)

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean

if optimized_compiler_builtins && !(is_rust_llvm() || compiler_rt_src_path)

?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sorry!

}

builder.update_submodule(&Path::new("src").join("llvm-project"));
let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
// Note that `libprofiler_builtins/build.rs` also computes this so if
// you're changing something here please also change that.
cargo.env("RUST_COMPILER_RT_ROOT", &compiler_builtins_root);
Expand Down
4 changes: 4 additions & 0 deletions src/bootstrap/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ pub struct Config {
pub color: Color,
pub patch_binaries_for_nix: bool,
pub stage0_metadata: Stage0Metadata,
/// Whether to use the `c` feature of the `compiler_builtins` crate.
pub optimized_compiler_builtins: bool,

pub on_fail: Option<String>,
pub stage: u32,
Expand Down Expand Up @@ -597,6 +599,7 @@ define_config! {
bench_stage: Option<u32> = "bench-stage",
patch_binaries_for_nix: Option<bool> = "patch-binaries-for-nix",
metrics: Option<bool> = "metrics",
optimized_compiler_builtins: Option<bool> = "optimized-compiler-builtins",
}
}

Expand Down Expand Up @@ -916,6 +919,7 @@ impl Config {
set(&mut config.print_step_timings, build.print_step_timings);
set(&mut config.print_step_rusage, build.print_step_rusage);
set(&mut config.patch_binaries_for_nix, build.patch_binaries_for_nix);
set(&mut config.optimized_compiler_builtins, build.optimized_compiler_builtins);

config.verbose = cmp::max(config.verbose, flags.verbose);

Expand Down
32 changes: 15 additions & 17 deletions src/bootstrap/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1838,23 +1838,21 @@ fn add_env(builder: &Builder<'_>, cmd: &mut Command, target: TargetSelection) {
///
/// 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 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
// the linker's load path at runtime), but the common use case for
// external LLVMs is distribution provided LLVMs, and in that case
// they're usually in the standard search path (e.g., /usr/lib) and
// copying them here is going to cause problems as we may end up
// with the wrong files and isn't what distributions want.
//
// This behavior may be revisited in the future though.
//
// If the LLVM is coming from ourselves (just from CI) though, we
// still want to install it, as it otherwise won't be available.
return false;
}
if !builder.is_rust_llvm(target) {
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this. The point of is_rust_llvm is to know whether it contains Rust patches. In #101072 I added a config option so you can use a vanilla upstream LLVM without affecting other parts of how bootstrap works, this breaks that by re-coupling the idea of "is patched" to "is in tree".

// 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
// the linker's load path at runtime), but the common use case for
// external LLVMs is distribution provided LLVMs, and in that case
// they're usually in the standard search path (e.g., /usr/lib) and
// copying them here is going to cause problems as we may end up
// with the wrong files and isn't what distributions want.
//
// This behavior may be revisited in the future though.
//
// If the LLVM is coming from ourselves (just from CI) though, we
// still want to install it, as it otherwise won't be available.
return false;
}

// On macOS, rustc (and LLVM tools) link to an unversioned libLLVM.dylib
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,6 @@ ENV RUST_CONFIGURE_ARGS --disable-jemalloc \
--set=$TARGET.cc=x86_64-unknown-haiku-gcc \
--set=$TARGET.cxx=x86_64-unknown-haiku-g++ \
--set=$TARGET.llvm-config=/bin/llvm-config-haiku
ENV EXTERNAL_LLVM 1

ENV SCRIPT python3 ../x.py dist --host=$HOST --target=$HOST
2 changes: 2 additions & 0 deletions src/ci/docker/host-x86_64/dist-various-2/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,6 @@ ENV RUST_CONFIGURE_ARGS --enable-extended --enable-lld --disable-docs \
--set target.wasm32-wasi.wasi-root=/wasm32-wasi \
--musl-root-armv7=/musl-armv7

ENV EXTERNAL_LLVM 1

ENV SCRIPT python3 ../x.py dist --host='' --target $TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ RUN sh /scripts/sccache.sh
# We are disabling CI LLVM since this builder is intentionally using a host
# LLVM, rather than the typical src/llvm-project LLVM.
ENV NO_DOWNLOAD_CI_LLVM 1
ENV EXTERNAL_LLVM 1

# Using llvm-link-shared due to libffi issues -- see #34486
ENV RUST_CONFIGURE_ARGS \
Expand Down
1 change: 1 addition & 0 deletions src/ci/docker/host-x86_64/x86_64-gnu-llvm-13/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ RUN sh /scripts/sccache.sh
# We are disabling CI LLVM since this builder is intentionally using a host
# LLVM, rather than the typical src/llvm-project LLVM.
ENV NO_DOWNLOAD_CI_LLVM 1
ENV EXTERNAL_LLVM 1

# Using llvm-link-shared due to libffi issues -- see #34486
ENV RUST_CONFIGURE_ARGS \
Expand Down
5 changes: 5 additions & 0 deletions src/ci/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.codegen-units-std=1"
# space required for CI artifacts.
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --dist-compression-formats=xz"

# Enable the `c` feature for compiler_builtins, but only when the `compiler-rt` source is available.
if [ "$EXTERNAL_LLVM" = "" ]; then
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.optimized-compiler-builtins"
fi

if [ "$DIST_SRC" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --disable-dist-src"
fi
Expand Down