From 5b374631df79f879560f3a4d041290e152c05ab0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 10 Aug 2024 15:02:13 +0000 Subject: [PATCH 1/8] compiletest: implement `needs-lvm-zstd` directive --- src/tools/compiletest/src/command-list.rs | 1 + src/tools/compiletest/src/header.rs | 101 ++++++++++++++++++++++ src/tools/compiletest/src/header/needs.rs | 10 ++- 3 files changed, 111 insertions(+), 1 deletion(-) diff --git a/src/tools/compiletest/src/command-list.rs b/src/tools/compiletest/src/command-list.rs index 7f8080235c870..a559d6f81a240 100644 --- a/src/tools/compiletest/src/command-list.rs +++ b/src/tools/compiletest/src/command-list.rs @@ -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", diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 1fc24301c85e6..933913eb47c34 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -1203,6 +1203,107 @@ pub fn extract_llvm_version_from_binary(binary_path: &str) -> Option { 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(()); + } + } + + 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 " [- ]", /// returns the numeric representation of and as /// tuple: ( as u32, as u32) diff --git a/src/tools/compiletest/src/header/needs.rs b/src/tools/compiletest/src/header/needs.rs index 8f935d5b74441..e04abea0bf987 100644 --- a/src/tools/compiletest/src/header/needs.rs +++ b/src/tools/compiletest/src/header/needs.rs @@ -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, @@ -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([':', ' ']) { @@ -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 { @@ -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(), } From 79f3c51a01693b264419f74b5527c146b4d52e81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 10 Aug 2024 15:09:20 +0000 Subject: [PATCH 2/8] mark `rust-lld-compress-debug-sections` test as needing zstd also make it fail if there's a compression issue --- .../rust-lld-compress-debug-sections/rmake.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/tests/run-make/rust-lld-compress-debug-sections/rmake.rs b/tests/run-make/rust-lld-compress-debug-sections/rmake.rs index ea4997fab8099..9889659046ea7 100644 --- a/tests/run-make/rust-lld-compress-debug-sections/rmake.rs +++ b/tests/run-make/rust-lld-compress-debug-sections/rmake.rs @@ -1,12 +1,13 @@ // Checks the `compress-debug-sections` option on rust-lld. //@ needs-rust-lld +//@ needs-llvm-zstd //@ only-linux //@ ignore-cross-compile // FIXME: This test isn't comprehensive and isn't covering all possible combinations. -use run_make_support::{assert_contains, llvm_readobj, run_in_tmpdir, rustc}; +use run_make_support::{llvm_readobj, run_in_tmpdir, rustc}; fn check_compression(compression: &str, to_find: &str) { run_in_tmpdir(|| { @@ -17,19 +18,8 @@ fn check_compression(compression: &str, to_find: &str) { .arg("-Cdebuginfo=full") .link_arg(&format!("-Wl,--compress-debug-sections={compression}")) .input("main.rs") - .run_unchecked(); - let stderr = out.stderr_utf8(); - if stderr.is_empty() { - llvm_readobj().arg("-t").arg("main").run().assert_stdout_contains(to_find); - } else { - assert_contains( - stderr, - format!( - "LLVM was not built with LLVM_ENABLE_{to_find} \ - or did not find {compression} at build time" - ), - ); - } + .run(); + llvm_readobj().arg("-t").arg("main").run().assert_stdout_contains(to_find); }); } From 802222fefca2e374a254877c54beb051843cd974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 10 Aug 2024 16:22:04 +0000 Subject: [PATCH 3/8] prepare test for expanding scope --- .../main.rs | 0 .../rmake.rs | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/run-make/{rust-lld-compress-debug-sections => compressed-debuginfo-zstd}/main.rs (100%) rename tests/run-make/{rust-lld-compress-debug-sections => compressed-debuginfo-zstd}/rmake.rs (100%) diff --git a/tests/run-make/rust-lld-compress-debug-sections/main.rs b/tests/run-make/compressed-debuginfo-zstd/main.rs similarity index 100% rename from tests/run-make/rust-lld-compress-debug-sections/main.rs rename to tests/run-make/compressed-debuginfo-zstd/main.rs diff --git a/tests/run-make/rust-lld-compress-debug-sections/rmake.rs b/tests/run-make/compressed-debuginfo-zstd/rmake.rs similarity index 100% rename from tests/run-make/rust-lld-compress-debug-sections/rmake.rs rename to tests/run-make/compressed-debuginfo-zstd/rmake.rs From 1935e21029d28af3022064f57b36fff85b8db9d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 10 Aug 2024 16:59:00 +0000 Subject: [PATCH 4/8] expand zstd debuginfo compression test it now checks zlib and zstd, via rustc and rust-lld --- .../compressed-debuginfo-zstd/rmake.rs | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/run-make/compressed-debuginfo-zstd/rmake.rs b/tests/run-make/compressed-debuginfo-zstd/rmake.rs index 9889659046ea7..8356373e949aa 100644 --- a/tests/run-make/compressed-debuginfo-zstd/rmake.rs +++ b/tests/run-make/compressed-debuginfo-zstd/rmake.rs @@ -1,24 +1,37 @@ -// Checks the `compress-debug-sections` option on rust-lld. +// Checks debuginfo compression both for the always-enabled zlib, and when the optional zstd is +// enabled: +// - via rustc's `debuginfo-compression`, +// - and via rust-lld's `compress-debug-sections` -//@ needs-rust-lld -//@ needs-llvm-zstd +//@ needs-llvm-zstd: we want LLVM/LLD to be built with zstd support +//@ needs-rust-lld: the system linker will most likely not support zstd //@ only-linux //@ ignore-cross-compile -// FIXME: This test isn't comprehensive and isn't covering all possible combinations. - -use run_make_support::{llvm_readobj, run_in_tmpdir, rustc}; +use run_make_support::{llvm_readobj, run_in_tmpdir, Rustc}; fn check_compression(compression: &str, to_find: &str) { + // check compressed debug sections via rustc flag + prepare_and_check(to_find, |rustc| { + rustc.arg(&format!("-Zdebuginfo-compression={compression}")) + }); + + // check compressed debug sections via rust-lld flag + prepare_and_check(to_find, |rustc| { + rustc.link_arg(&format!("-Wl,--compress-debug-sections={compression}")) + }); +} + +fn prepare_and_check &mut Rustc>(to_find: &str, prepare_rustc: F) { run_in_tmpdir(|| { - let out = rustc() + let mut rustc = Rustc::new(); + rustc .arg("-Zlinker-features=+lld") .arg("-Clink-self-contained=+linker") .arg("-Zunstable-options") .arg("-Cdebuginfo=full") - .link_arg(&format!("-Wl,--compress-debug-sections={compression}")) - .input("main.rs") - .run(); + .input("main.rs"); + prepare_rustc(&mut rustc).run(); llvm_readobj().arg("-t").arg("main").run().assert_stdout_contains(to_find); }); } From 62c8c693bdda3932dbb049f8a2707f2f616d29eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 10 Aug 2024 22:25:22 +0000 Subject: [PATCH 5/8] move and rename zstd script move it where it's used, and name it like the other scripts --- src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile | 6 +++--- .../zstd.sh => host-x86_64/dist-x86_64-linux/build-zstd.sh} | 0 2 files changed, 3 insertions(+), 3 deletions(-) rename src/ci/docker/{scripts/zstd.sh => host-x86_64/dist-x86_64-linux/build-zstd.sh} (100%) diff --git a/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile b/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile index 61e9694f1e2ae..e857f38e68a85 100644 --- a/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile +++ b/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile @@ -62,9 +62,9 @@ COPY host-x86_64/dist-x86_64-linux/build-clang.sh /tmp/ RUN ./build-clang.sh ENV CC=clang CXX=clang++ -# rustc's LLVM needs zstd. -COPY scripts/zstd.sh /tmp/ -RUN ./zstd.sh +# Build zstd to enable `llvm.libzstd`. +COPY host-x86_64/dist-x86_64-linux/build-zstd.sh /tmp/ +RUN ./build-zstd.sh COPY scripts/sccache.sh /scripts/ RUN sh /scripts/sccache.sh diff --git a/src/ci/docker/scripts/zstd.sh b/src/ci/docker/host-x86_64/dist-x86_64-linux/build-zstd.sh similarity index 100% rename from src/ci/docker/scripts/zstd.sh rename to src/ci/docker/host-x86_64/dist-x86_64-linux/build-zstd.sh From 7963beda2481deb5bd01286b61386d599536a3b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sun, 11 Aug 2024 15:02:13 +0000 Subject: [PATCH 6/8] strip whitespace for ignored tests reason comments --- src/tools/compiletest/src/header/needs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/compiletest/src/header/needs.rs b/src/tools/compiletest/src/header/needs.rs index e04abea0bf987..72b1b9c6d480a 100644 --- a/src/tools/compiletest/src/header/needs.rs +++ b/src/tools/compiletest/src/header/needs.rs @@ -174,7 +174,7 @@ pub(super) fn handle_needs( } else { return IgnoreDecision::Ignore { reason: if let Some(comment) = comment { - format!("{} ({comment})", need.ignore_reason) + format!("{} ({})", need.ignore_reason, comment.trim()) } else { need.ignore_reason.into() }, From 650ba7fdf001353664a60be43efb2d96c3c0295b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sun, 11 Aug 2024 19:05:30 +0000 Subject: [PATCH 7/8] enable `llvm.libzstd` on test x64 linux builder --- src/ci/docker/host-x86_64/x86_64-gnu/Dockerfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ci/docker/host-x86_64/x86_64-gnu/Dockerfile b/src/ci/docker/host-x86_64/x86_64-gnu/Dockerfile index 19683317126ab..83c2aa8cfb3b7 100644 --- a/src/ci/docker/host-x86_64/x86_64-gnu/Dockerfile +++ b/src/ci/docker/host-x86_64/x86_64-gnu/Dockerfile @@ -28,5 +28,6 @@ ENV RUST_CONFIGURE_ARGS \ --build=x86_64-unknown-linux-gnu \ --enable-sanitizers \ --enable-profiler \ - --enable-compiler-docs + --enable-compiler-docs \ + --set llvm.libzstd=true ENV SCRIPT python3 ../x.py --stage 2 test From 5d83cb27c886137887c087fbce77032069f5ec8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 20 Aug 2024 14:04:44 +0000 Subject: [PATCH 8/8] allow `llvm.libzstd` with `download-ci-llvm = true` but warn about it --- src/bootstrap/src/core/config/config.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index ce23b7735f8bd..bdfee55d8d183 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1885,6 +1885,22 @@ impl Config { warn("link-shared"); } + // FIXME(#129153): instead of all the ad-hoc `download-ci-llvm` checks that follow, + // use the `builder-config` present in tarballs since #128822 to compare the local + // config to the ones used to build the LLVM artifacts on CI, and only notify users + // if they've chosen a different value. + + if libzstd.is_some() { + println!( + "WARNING: when using `download-ci-llvm`, the local `llvm.libzstd` option, \ + like almost all `llvm.*` options, will be ignored and set by the LLVM CI \ + artifacts builder config." + ); + println!( + "HELP: To use `llvm.libzstd` for LLVM/LLD builds, set `download-ci-llvm` option to false." + ); + } + // None of the LLVM options, except assertions, are supported // when using downloaded LLVM. We could just ignore these but // that's potentially confusing, so force them to not be @@ -1894,7 +1910,6 @@ impl Config { check_ci_llvm!(optimize_toml); check_ci_llvm!(thin_lto); check_ci_llvm!(release_debuginfo); - check_ci_llvm!(libzstd); check_ci_llvm!(targets); check_ci_llvm!(experimental_targets); check_ci_llvm!(clang_cl);