From 03abf6756ded764648c500d39e506d34ca30868b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 3 Oct 2024 14:27:47 +0000 Subject: [PATCH 1/2] Use untracked env var to pass `-Zon-broken-pipe=kill` for tools - Don't touch rustc's `-Zon-broken-pipe=kill` env var in `compile.rs`. - Use an untracked env var to pass `-Zon-broken-pipe=kill` for tools but skip cargo still, because cargo wants `-Zon-broken-pipe=kill` unset. --- src/bootstrap/src/bin/rustc.rs | 8 ++++++ src/bootstrap/src/core/build_steps/compile.rs | 15 +++++++++-- src/bootstrap/src/core/build_steps/tool.rs | 25 ++++++++++++++++--- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/bootstrap/src/bin/rustc.rs b/src/bootstrap/src/bin/rustc.rs index 780979ed981..6f7ccde5e5b 100644 --- a/src/bootstrap/src/bin/rustc.rs +++ b/src/bootstrap/src/bin/rustc.rs @@ -136,6 +136,14 @@ fn main() { cmd.args(lint_flags.split_whitespace()); } + // Conditionally pass `-Zon-broken-pipe=kill` to rustc bin shim when this shim is *not* used to + // build cargo itself, i.e. set `-Zon-broken-pipe=kill` only when building non-cargo tools. + // + // See for more context. + if env::var_os("FORCE_ON_BROKEN_PIPE_KILL").is_some() { + cmd.arg("-Z").arg("on-broken-pipe=kill"); + } + if target.is_some() { // The stage0 compiler has a special sysroot distinct from what we // actually downloaded, so we just always pass the `--sysroot` option, diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index b5be7d841dd..27bbc8bd8ff 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -1053,8 +1053,19 @@ pub fn rustc_cargo( cargo.rustdocflag("-Zcrate-attr=warn(rust_2018_idioms)"); - // If the rustc output is piped to e.g. `head -n1` we want the process to be - // killed, rather than having an error bubble up and cause a panic. + // If the rustc output is piped to e.g. `head -n1` we want the process to be killed, rather than + // having an error bubble up and cause a panic. + // + // FIXME(jieyouxu): this flag is load-bearing for rustc to not ICE on broken pipes, because + // rustc internally sometimes uses std `println!` -- but std `println!` by default will panic on + // broken pipes, and uncaught panics will manifest as an ICE. The compiler *should* handle this + // properly, but this flag is set in the meantime to paper over the I/O errors. + // + // See for details. + // + // Also see the discussion for properly handling I/O errors related to broken pipes, i.e. safe + // variants of `println!` in + // . cargo.rustflag("-Zon-broken-pipe=kill"); if builder.config.llvm_enzyme { diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index e2fcd13efe3..a01497c2bb9 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -209,11 +209,28 @@ pub fn prepare_tool_cargo( // See https://github.com/rust-lang/rust/issues/116538 cargo.rustflag("-Zunstable-options"); - // `-Zon-broken-pipe=kill` breaks cargo tests + // NOTE: The root cause of needing `-Zon-broken-pipe=kill` in the first place is because `rustc` + // and `rustdoc` doesn't gracefully handle I/O errors due to usages of raw std `println!` macros + // which panics upon encountering broken pipes. `-Zon-broken-pipe=kill` just papers over that + // and stops rustc/rustdoc ICEing on e.g. `rustc --print=sysroot | false`. + // + // cargo explicitly does not want the `-Zon-broken-pipe=kill` paper because it does actually use + // variants of `println!` that handles I/O errors gracefully. It's also a breaking change for a + // spawn process not written in Rust, especially if the language default handler is not + // `SIG_IGN`. Thankfully cargo tests will break if we do set the flag. + // + // For the cargo discussion, see + // . + // + // For the rustc discussion, see + // + // for proper solutions. if !path.ends_with("cargo") { - // If the output is piped to e.g. `head -n1` we want the process to be killed, - // rather than having an error bubble up and cause a panic. - cargo.rustflag("-Zon-broken-pipe=kill"); + // Use an untracked env var `FORCE_ON_BROKEN_PIPE_KILL` here instead of `RUSTFLAGS`. + // `RUSTFLAGS` is tracked by cargo. Conditionally omitting `-Zon-broken-pipe=kill` from + // `RUSTFLAGS` causes unnecessary tool rebuilds due to cache invalidation from building e.g. + // cargo *without* `-Zon-broken-pipe=kill` but then rustdoc *with* `-Zon-broken-pipe=kill`. + cargo.env("FORCE_ON_BROKEN_PIPE_KILL", "-Zon-broken-pipe=kill"); } cargo From 5c0131641d2edfc21e4f627f94f0aadc6cfc6e47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 3 Oct 2024 16:27:11 +0000 Subject: [PATCH 2/2] Add regression test for rustc/rustdoc broken pipe ICEs --- src/bootstrap/src/bin/rustc.rs | 6 +- tests/run-make/broken-pipe-no-ice/rmake.rs | 73 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 tests/run-make/broken-pipe-no-ice/rmake.rs diff --git a/src/bootstrap/src/bin/rustc.rs b/src/bootstrap/src/bin/rustc.rs index 6f7ccde5e5b..18f5a1a58db 100644 --- a/src/bootstrap/src/bin/rustc.rs +++ b/src/bootstrap/src/bin/rustc.rs @@ -136,10 +136,8 @@ fn main() { cmd.args(lint_flags.split_whitespace()); } - // Conditionally pass `-Zon-broken-pipe=kill` to rustc bin shim when this shim is *not* used to - // build cargo itself, i.e. set `-Zon-broken-pipe=kill` only when building non-cargo tools. - // - // See for more context. + // Conditionally pass `-Zon-broken-pipe=kill` to underlying rustc. Not all binaries want + // `-Zon-broken-pipe=kill`, which includes cargo itself. if env::var_os("FORCE_ON_BROKEN_PIPE_KILL").is_some() { cmd.arg("-Z").arg("on-broken-pipe=kill"); } diff --git a/tests/run-make/broken-pipe-no-ice/rmake.rs b/tests/run-make/broken-pipe-no-ice/rmake.rs new file mode 100644 index 00000000000..d1db0bc7368 --- /dev/null +++ b/tests/run-make/broken-pipe-no-ice/rmake.rs @@ -0,0 +1,73 @@ +//! Check that `rustc` and `rustdoc` does not ICE upon encountering a broken pipe due to unhandled +//! panics from raw std `println!` usages. +//! +//! Regression test for . + +//@ ignore-cross-compile (needs to run test binary) + +#![feature(anonymous_pipe)] + +use std::io::Read; +use std::process::{Command, Stdio}; + +use run_make_support::env_var; + +#[derive(Debug, PartialEq)] +enum Binary { + Rustc, + Rustdoc, +} + +fn check_broken_pipe_handled_gracefully(bin: Binary, mut cmd: Command) { + let (reader, writer) = std::pipe::pipe().unwrap(); + drop(reader); // close read-end + cmd.stdout(writer).stderr(Stdio::piped()); + + let mut child = cmd.spawn().unwrap(); + + let mut stderr = String::new(); + child.stderr.as_mut().unwrap().read_to_string(&mut stderr).unwrap(); + let status = child.wait().unwrap(); + + assert!(!status.success(), "{bin:?} unexpectedly succeeded"); + + const PANIC_ICE_EXIT_CODE: i32 = 101; + + #[cfg(not(windows))] + { + // On non-Windows, rustc/rustdoc built with `-Zon-broken-pipe=kill` shouldn't have an exit + // code of 101 because it should have an wait status that corresponds to SIGPIPE signal + // number. + assert_ne!(status.code(), Some(PANIC_ICE_EXIT_CODE), "{bin:?}"); + // And the stderr should be empty because rustc/rustdoc should've gotten killed. + assert!(stderr.is_empty(), "{bin:?} stderr:\n{}", stderr); + } + + #[cfg(windows)] + { + match bin { + // On Windows, rustc has a paper that propagates the panic exit code of 101 but converts + // broken pipe errors into fatal errors instead of ICEs. + Binary::Rustc => { + assert_eq!(status.code(), Some(PANIC_ICE_EXIT_CODE), "{bin:?}"); + // But make sure it doesn't manifest as an ICE. + assert!(!stderr.contains("internal compiler error"), "{bin:?} ICE'd"); + } + // On Windows, rustdoc seems to cleanly exit with exit code of 1. + Binary::Rustdoc => { + assert_eq!(status.code(), Some(1), "{bin:?}"); + assert!(!stderr.contains("panic"), "{bin:?} stderr contains panic"); + } + } + } +} + +fn main() { + let mut rustc = Command::new(env_var("RUSTC")); + rustc.arg("--print=sysroot"); + check_broken_pipe_handled_gracefully(Binary::Rustc, rustc); + + let mut rustdoc = Command::new(env_var("RUSTDOC")); + rustdoc.arg("--version"); + check_broken_pipe_handled_gracefully(Binary::Rustdoc, rustdoc); +}