-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) { | ||
panic!( | ||
"need a managed LLVM submodule for optimized intrinsics support; unset `llvm-config` or `optimized-compiler-builtins`" | ||
); | ||
Comment on lines
+315
to
+317
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 What I would do is have a check like: if optimized_compiler_builtins && (!is_rust_llvm() || compiler_rt_src_path) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this. The point of |
||
// 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 | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 insrc/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.There was a problem hiding this comment.
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