Skip to content

Commit

Permalink
Rollup merge of #103732 - Mark-Simulacrum:revert-compiler-builtins, r…
Browse files Browse the repository at this point in the history
…=jyn514

Revert "Make the `c` feature for `compiler-builtins` opt-in instead of inferred"

This reverts commit 3acb505 (PR #101833).

The changes in this commit caused several bugs/incompatibilities (#101833 (comment), #102560). For now we're reverting this commit and will re-land it alongside fixes for those bugs.

Re-opens #101172
cc #102560
cc #102579
  • Loading branch information
Dylan-DPC authored Oct 31, 2022
2 parents c1c2922 + 5984b1d commit 3affb05
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 44 deletions.
4 changes: 0 additions & 4 deletions config.toml.example
Original file line number Diff line number Diff line change
Expand Up @@ -291,10 +291,6 @@ 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: 5 additions & 10 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ 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.
// `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).
//
// 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 @@ -310,15 +312,8 @@ 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_c_feature = if builder.config.optimized_compiler_builtins {
if !builder.is_rust_llvm(target) {
panic!(
"need a managed LLVM submodule for optimized intrinsics support; unset `llvm-config` or `optimized-compiler-builtins`"
);
}

builder.update_submodule(&Path::new("src").join("llvm-project"));
let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
let compiler_builtins_c_feature = if compiler_builtins_root.exists() {
// 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: 0 additions & 4 deletions src/bootstrap/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ 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 @@ -624,7 +622,6 @@ 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 @@ -1013,7 +1010,6 @@ 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: 17 additions & 15 deletions src/bootstrap/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1847,21 +1847,23 @@ 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 !builder.is_rust_llvm(target) {
// 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 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;
}
}

// 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,6 +47,4 @@ 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: 0 additions & 2 deletions src/ci/docker/host-x86_64/dist-various-2/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,4 @@ 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,7 +29,6 @@ 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: 0 additions & 1 deletion 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,7 +40,6 @@ 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: 0 additions & 5 deletions src/ci/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ 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
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

0 comments on commit 3affb05

Please sign in to comment.