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

check host's libstdc++ version when using ci llvm #125411

Merged
merged 6 commits into from
Jun 6, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions src/bootstrap/src/core/build_steps/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::core::builder::{Builder, Cargo as CargoCommand, RunConfig, ShouldRun,
use crate::core::config::TargetSelection;
use crate::utils::channel::GitInfo;
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::output;
use crate::utils::helpers::{add_dylib_path, exe, t};
use crate::Compiler;
use crate::Mode;
Expand Down Expand Up @@ -804,6 +805,59 @@ impl Step for LlvmBitcodeLinker {
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
pub struct LibcxxVersionTool {
pub target: TargetSelection,
}

#[allow(dead_code)]
#[derive(Debug, Clone)]
pub enum LibcxxVersion {
Gnu(usize),
Llvm(usize),
}

impl Step for LibcxxVersionTool {
type Output = LibcxxVersion;
const DEFAULT: bool = false;
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.never()
}

fn run(self, builder: &Builder<'_>) -> LibcxxVersion {
let out_dir = builder.out.join(self.target.to_string()).join("libcxx-version");
let _ = fs::remove_dir_all(&out_dir);
t!(fs::create_dir_all(&out_dir));

let compiler = builder.cxx(self.target).unwrap();
let mut cmd = Command::new(compiler);

let executable = out_dir.join("libcxx-version");
Copy link
Contributor

Choose a reason for hiding this comment

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

@onur-ozkan

Suggested change
let executable = out_dir.join("libcxx-version");
let executable = out_dir.join("libcxx-version.exe");

How did it pass CI on Windows?
It trivially fails for me locally, because there's no libcxx-version, only libcxx-version.exe.

Copy link
Member Author

Choose a reason for hiding this comment

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

#126086 should handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it passed because windows-gnu CI uses env that disables llvm download?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it passed because windows-gnu CI uses env that disables llvm download?

Yeah, most likely.

cmd.arg("-o").arg(&executable).arg(builder.src.join("src/tools/libcxx-version/main.cpp"));

builder.run_cmd(&mut cmd);

if !executable.exists() {
panic!("Something went wrong. {} is not present", executable.display());
}

let version_output = output(&mut Command::new(executable));

let version_str = version_output.split_once("version:").unwrap().1;
let version = version_str.trim().parse::<usize>().unwrap();

if version_output.starts_with("libstdc++") {
LibcxxVersion::Gnu(version)
} else if version_output.starts_with("libc++") {
LibcxxVersion::Llvm(version)
} else {
panic!("Coudln't recognize the standard library version.");
}
}
}

macro_rules! tool_extended {
(($sel:ident, $builder:ident),
$($name:ident,
Expand Down
35 changes: 35 additions & 0 deletions src/bootstrap/src/core/sanity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ use std::fs;
use std::path::PathBuf;
use std::process::Command;

#[cfg(not(feature = "bootstrap-self-test"))]
use crate::builder::Builder;
#[cfg(not(feature = "bootstrap-self-test"))]
use crate::core::build_steps::tool;
#[cfg(not(feature = "bootstrap-self-test"))]
use std::collections::HashSet;

Expand All @@ -38,6 +42,11 @@ const STAGE0_MISSING_TARGETS: &[&str] = &[
// just a dummy comment so the list doesn't get onelined
];

/// Minimum version threshold for libstdc++ required when using prebuilt LLVM
/// from CI (with`llvm.download-ci-llvm` option).
#[cfg(not(feature = "bootstrap-self-test"))]
const LIBSTDCXX_MIN_VERSION_THRESHOLD: usize = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Is minimum really the right thing here? I'd expect that we need an exact match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless there is a breaking change, different versions can be compatible (just like libc). It doesn't need to be the same version.


impl Finder {
pub fn new() -> Self {
Self { cache: HashMap::new(), path: env::var_os("PATH").unwrap_or_default() }
Expand Down Expand Up @@ -102,6 +111,32 @@ pub fn check(build: &mut Build) {
cmd_finder.must_have("git");
}

// Ensure that a compatible version of libstdc++ is available on the system when using `llvm.download-ci-llvm`.
#[cfg(not(feature = "bootstrap-self-test"))]
if !build.config.dry_run() && !build.build.is_msvc() && build.config.llvm_from_ci {
let builder = Builder::new(build);
let libcxx_version = builder.ensure(tool::LibcxxVersionTool { target: build.build });

match libcxx_version {
tool::LibcxxVersion::Gnu(version) => {
if LIBSTDCXX_MIN_VERSION_THRESHOLD > version {
eprintln!(
"\nYour system's libstdc++ version is too old for the `llvm.download-ci-llvm` option."
);
eprintln!("Current version detected: '{}'", version);
eprintln!("Minimum required version: '{}'", LIBSTDCXX_MIN_VERSION_THRESHOLD);
eprintln!(
"Consider upgrading libstdc++ or disabling the `llvm.download-ci-llvm` option."
);
crate::exit!(1);
}
}
tool::LibcxxVersion::Llvm(_) => {
// FIXME: Handle libc++ version check.
}
}
}

// We need cmake, but only if we're actually building LLVM or sanitizers.
let building_llvm = build
.hosts
Expand Down
26 changes: 26 additions & 0 deletions src/tools/libcxx-version/main.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Detecting the standard library version manually using a bunch of shell commands is very
// complicated and fragile across different platforms. This program provides the major version
// of the standard library on any target platform without requiring any messy work.
//
// It's nothing more than specifying the name of the standard library implementation (either libstdc++ or libc++)
// and its major version.

#include <iostream>

int main() {
#ifdef _GLIBCXX_RELEASE
std::cout << "libstdc++ version: " << _GLIBCXX_RELEASE << std::endl;
#elif defined(_LIBCPP_VERSION)
// _LIBCPP_VERSION follows "XXYYZZ" format (e.g., 170001 for 17.0.1).
// ref: https://github.com/llvm/llvm-project/blob/f64732195c1030ee2627ff4e4142038e01df1d26/libcxx/include/__config#L51-L54
//
// Since we use the major version from _GLIBCXX_RELEASE, we need to extract only the first 2 characters of _LIBCPP_VERSION
// to provide the major version for consistency.
std::cout << "libc++ version: " << std::to_string(_LIBCPP_VERSION).substr(0, 2) << std::endl;
#else
std::cerr << "Coudln't recognize the standard library version." << std::endl;
return 1;
#endif

return 0;
}
1 change: 1 addition & 0 deletions src/tools/tidy/src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub fn filter_dirs(path: &Path) -> bool {
"library/stdarch",
"src/tools/cargo",
"src/tools/clippy",
"src/tools/libcxx-version",
"src/tools/miri",
"src/tools/rust-analyzer",
"src/tools/rustc-perf",
Expand Down
2 changes: 2 additions & 0 deletions triagebot.toml
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ trigger_files = [
"src/tools/compiletest",
"src/tools/tidy",
"src/tools/rustdoc-gui-test",
"src/tools/libcxx-version",
]

[autolabel."T-infra"]
Expand Down Expand Up @@ -1117,6 +1118,7 @@ project-exploit-mitigations = [
"/src/tools/tidy" = ["bootstrap"]
"/src/tools/x" = ["bootstrap"]
"/src/tools/rustdoc-gui-test" = ["bootstrap", "@onur-ozkan"]
"/src/tools/libcxx-version" = ["@onur-ozkan"]

# Enable tracking of PR review assignment
# Documentation at: https://forge.rust-lang.org/triagebot/pr-assignment-tracking.html
Expand Down
Loading