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

More work on zstd compression #128935

Merged
merged 8 commits into from
Aug 27, 2024
Merged
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
compiletest: implement needs-lvm-zstd directive
  • Loading branch information
lqd committed Aug 25, 2024
commit 5b374631df79f879560f3a4d041290e152c05ab0
1 change: 1 addition & 0 deletions src/tools/compiletest/src/command-list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"needs-force-clang-based-tests",
"needs-git-hash",
"needs-llvm-components",
"needs-llvm-zstd",
"needs-profiler-support",
"needs-relocation-model-pic",
"needs-run-enabled",
Expand Down
101 changes: 101 additions & 0 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,6 +1203,107 @@ pub fn extract_llvm_version_from_binary(binary_path: &str) -> Option<u32> {
None
}

/// For tests using the `needs-llvm-zstd` directive:
/// - for local LLVM builds, try to find the static zstd library in the llvm-config system libs.
/// - for `download-ci-llvm`, see if `lld` was built with zstd support.
pub fn llvm_has_libzstd(config: &Config) -> bool {
// Strategy 1: works for local builds but not with `download-ci-llvm`.
//
// We check whether `llvm-config` returns the zstd library. Bootstrap's `llvm.libzstd` will only
// ask to statically link it when building LLVM, so we only check if the list of system libs
// contains a path to that static lib, and that it exists.
//
// See compiler/rustc_llvm/build.rs for more details and similar expectations.
fn is_zstd_in_config(llvm_bin_dir: &Path) -> Option<()> {
let llvm_config_path = llvm_bin_dir.join("llvm-config");
let output = Command::new(llvm_config_path).arg("--system-libs").output().ok()?;
assert!(output.status.success(), "running llvm-config --system-libs failed");

let libs = String::from_utf8(output.stdout).ok()?;
for lib in libs.split_whitespace() {
if lib.ends_with("libzstd.a") && Path::new(lib).exists() {
return Some(());
}
}
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved

None
}

// Strategy 2: `download-ci-llvm`'s `llvm-config --system-libs` will not return any libs to
// use.
//
// The CI artifacts also don't contain the bootstrap config used to build them: otherwise we
// could have looked at the `llvm.libzstd` config.
//
// We infer whether `LLVM_ENABLE_ZSTD` was used to build LLVM as a byproduct of testing whether
// `lld` supports it. If not, an error will be emitted: "LLVM was not built with
// LLVM_ENABLE_ZSTD or did not find zstd at build time".
#[cfg(unix)]
fn is_lld_built_with_zstd(llvm_bin_dir: &Path) -> Option<()> {
let lld_path = llvm_bin_dir.join("lld");
if lld_path.exists() {
// We can't call `lld` as-is, it expects to be invoked by a compiler driver using a
// different name. Prepare a temporary symlink to do that.
let lld_symlink_path = llvm_bin_dir.join("ld.lld");
if !lld_symlink_path.exists() {
std::os::unix::fs::symlink(lld_path, &lld_symlink_path).ok()?;
}

// Run `lld` with a zstd flag. We expect this command to always error here, we don't
// want to link actual files and don't pass any.
let output = Command::new(&lld_symlink_path)
.arg("--compress-debug-sections=zstd")
.output()
.ok()?;
assert!(!output.status.success());

// Look for a specific error caused by LLVM not being built with zstd support. We could
// also look for the "no input files" message, indicating the zstd flag was accepted.
let stderr = String::from_utf8(output.stderr).ok()?;
let zstd_available = !stderr.contains("LLVM was not built with LLVM_ENABLE_ZSTD");

// We don't particularly need to clean the link up (so the previous commands could fail
// in theory but won't in practice), but we can try.
std::fs::remove_file(lld_symlink_path).ok()?;

if zstd_available {
return Some(());
}
}

None
}

#[cfg(not(unix))]
fn is_lld_built_with_zstd(_llvm_bin_dir: &Path) -> Option<()> {
None
}

if let Some(llvm_bin_dir) = &config.llvm_bin_dir {
// Strategy 1: for local LLVM builds.
if is_zstd_in_config(llvm_bin_dir).is_some() {
return true;
}

// Strategy 2: for LLVM artifacts built on CI via `download-ci-llvm`.
//
// It doesn't work for cases where the artifacts don't contain the linker, but it's
// best-effort: CI has `llvm.libzstd` and `lld` enabled on the x64 linux artifacts, so it
// will at least work there.
//
// If this can be improved and expanded to less common cases in the future, it should.
if config.target == "x86_64-unknown-linux-gnu"
&& config.host == config.target
&& is_lld_built_with_zstd(llvm_bin_dir).is_some()
{
return true;
}
}

// Otherwise, all hope is lost.
false
}

/// Takes a directive of the form "<version1> [- <version2>]",
/// returns the numeric representation of <version1> and <version2> as
/// tuple: (<version1> as u32, <version2> as u32)
Expand Down
10 changes: 9 additions & 1 deletion src/tools/compiletest/src/header/needs.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::common::{Config, Sanitizer};
use crate::header::IgnoreDecision;
use crate::header::{llvm_has_libzstd, IgnoreDecision};

pub(super) fn handle_needs(
cache: &CachedNeedsConditions,
Expand Down Expand Up @@ -144,6 +144,11 @@ pub(super) fn handle_needs(
condition: cache.symlinks,
ignore_reason: "ignored if symlinks are unavailable",
},
Need {
name: "needs-llvm-zstd",
condition: cache.llvm_zstd,
ignore_reason: "ignored if LLVM wasn't build with zstd for ELF section compression",
},
];

let (name, comment) = match ln.split_once([':', ' ']) {
Expand Down Expand Up @@ -210,6 +215,8 @@ pub(super) struct CachedNeedsConditions {
rust_lld: bool,
dlltool: bool,
symlinks: bool,
/// Whether LLVM built with zstd, for the `needs-llvm-zstd` directive.
llvm_zstd: bool,
}

impl CachedNeedsConditions {
Expand Down Expand Up @@ -253,6 +260,7 @@ impl CachedNeedsConditions {
.join(if config.host.contains("windows") { "rust-lld.exe" } else { "rust-lld" })
.exists(),

llvm_zstd: llvm_has_libzstd(&config),
dlltool: find_dlltool(&config),
symlinks: has_symlinks(),
}
Expand Down