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

Reland optimized-compiler-builtins config #102579

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
add and document a new is_system_llvm abstraction
  • Loading branch information
jyn514 committed Dec 25, 2023
commit b3ebe50664bbc5be6db6dc6ddb320e28105d37ba
35 changes: 18 additions & 17 deletions src/bootstrap/src/core/build_steps/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2042,23 2042,24 @@ 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 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 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.
//
// NOTE: this intentionally doesn't use `is_rust_llvm`; whether this is patched or not doesn't matter,
// we only care if the shared object itself is managed by bootstrap.
//
// If the LLVM is coming from ourselves (just from CI) though, we
// still want to install it, as it otherwise won't be available.
if builder.is_system_llvm(target) {
return false;
}

// On macOS, rustc (and LLVM tools) link to an unversioned libLLVM.dylib
Expand Down
2 changes: 2 additions & 0 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1848,6 1848,8 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
llvm_components_passed = true;
}
if !builder.is_rust_llvm(target) {
// FIXME: missing Rust patches is not the same as being system llvm; we should rename the flag at some point.
// Inspecting the tests with `// no-system-llvm` in src/test *looks* like this is doing the right thing, though.
cmd.arg("--system-llvm");
}

Expand Down
9 changes: 8 additions & 1 deletion src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1810,7 1810,14 @@ impl Config {
}
target.llvm_config = Some(config.src.join(s));
}
target.llvm_has_rust_patches = cfg.llvm_has_rust_patches;
if let Some(patches) = cfg.llvm_has_rust_patches {
assert_eq!(
config.submodules,
Some(false),
"cannot set `llvm-has-rust-patches` for a managed submodule (set `build.submodules = false` if you want to apply patches)"
);
target.llvm_has_rust_patches = Some(patches);
}
if let Some(ref s) = cfg.llvm_filecheck {
target.llvm_filecheck = Some(config.src.join(s));
}
Expand Down
32 changes: 24 additions & 8 deletions src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,18 823,34 @@ impl Build {
INTERNER.intern_path(self.out.join(&*target.triple).join("md-doc"))
}

/// Returns `true` if no custom `llvm-config` is set for the specified target.
/// Returns `true` if this is an external version of LLVM not managed by bootstrap.
/// In particular, we expect llvm sources to be available when this is false.
///
/// If no custom `llvm-config` was specified then Rust's llvm will be used.
/// NOTE: this is not the same as `!is_rust_llvm` when `llvm_has_patches` is set.
fn is_system_llvm(&self, target: TargetSelection) -> bool {
match self.config.target_config.get(&target) {
Some(Target { llvm_config: Some(_), .. }) => {
let ci_llvm = self.config.llvm_from_ci && target == self.config.build;
!ci_llvm
}
// We're building from the in-tree src/llvm-project sources.
Some(Target { llvm_config: None, .. }) => false,
None => false,
}
}

/// Returns `true` if this is our custom, patched, version of LLVM.
///
/// This does not necessarily imply that we're managing the `llvm-project` submodule.
fn is_rust_llvm(&self, target: TargetSelection) -> bool {
match self.config.target_config.get(&target) {
// We're using a user-controlled version of LLVM. The user has explicitly told us whether the version has our patches.
// (They might be wrong, but that's not a supported use-case.)
// In particular, this tries to support `submodules = false` and `patches = false`, for using a newer version of LLVM that's not through `rust-lang/llvm-project`.
Some(Target { llvm_has_rust_patches: Some(patched), .. }) => *patched,
Some(Target { llvm_config, .. }) => {
// If the user set llvm-config we assume Rust is not patched,
// but first check to see if it was configured by llvm-from-ci.
(self.config.llvm_from_ci && target == self.config.build) || llvm_config.is_none()
}
None => true,
// The user hasn't promised the patches match.
// This only has our patches if it's downloaded from CI or built from source.
_ => !self.is_system_llvm(target),
}
}

Expand Down