Skip to content

Commit

Permalink
Auto merge of #131155 - jieyouxu:always-kill, r=onur-ozkan
Browse files Browse the repository at this point in the history
Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS`

This PR fixes #130980 where building cargo invalidated the tool build caches of other tools (such as rustdoc) because `-Zon-broken-pipe=kill` was conditionally passed via `RUSTFLAGS` for other tools *except* for cargo. The differing `RUSTFLAGS` triggered tool build cache invalidation as `RUSTFLAGS` is a tracked env var -- any changes in `RUSTFLAGS` requires a rebuild.

`-Zon-broken-pipe=kill` is load-bearing for rustc and rustdoc to not ICE on broken pipes due to usages of raw std `println!` that panics without the flag being set, which manifests in ICEs.

I can't say I like the changes here, but it is what it is...

See detailed discussions and history of `-Zon-broken-pipe=kill` usage in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F/near/474593815.

## Approach

This PR fixes the tool build cache invalidation by informing the `rustc` binary shim when to apply `-Zon-broken-pipe=kill` (i.e. when the rustc binary shim is not used to build cargo). This information is not communicated by `RUSTFLAGS`, which is an env var tracked by cargo, and instead uses an untracked env var `UNTRACKED_BROKEN_PIPE_FLAG` so we won't trigger tool build cache invalidation. We preserve bootstrap's behavior of not setting that flag for cargo by conditionally omitting setting `UNTRACKED_BROKEN_PIPE_FLAG` when building cargo.

Notably, the `-Zon-broken-pipe=kill` instance in https://github.com/rust-lang/rust/blob/1e5719bdc40bb553089ce83525f07dfe0b2e71e9/src/bootstrap/src/core/build_steps/compile.rs#L1058 is not modified because that is used to build rustc only and not cargo itself.

Thanks to `@cuviper` for the idea!

## Testing

### Integration testing

This PR introduces a run-make test for rustc and rustdoc that checks that when they do not ICE/panic when they encounter a broken pipe of the stdout stream.

I checked this test will catch the broken pipe ICE regression for rustc on Linux (at least) by commenting out https://github.com/rust-lang/rust/blob/1e5719bdc40bb553089ce83525f07dfe0b2e71e9/src/bootstrap/src/core/build_steps/compile.rs#L1058, and the test failed because rustc ICE'd.

### Manual testing

I have manually tried:

1. `./x clean && `./x test build --stage 1` -> `rustc +stage1 --print=sysroot | false`: no ICE.
2. `./x clean` -> `./x test run-make` twice: no stage 1 cargo rebuilds.
3. `./x clean` -> `./x build rustdoc` -> `rustdoc +stage1 --version | false`: no panics.
4. `./x test src/tools/cargo`: tests pass, notably `build::close_output` and `cargo_command::closed_output_ok` do not fail which would fail if cargo was built with `-Zon-broken-pipe=kill`.

## Related discussions

Thanks to everyone who helped!
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Applying.20.60-Zon-broken-pipe.3Dkill.60.20flags.20in.20bootstrap.3F
- https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Modifying.20run-make.20tests.20unnecessarily.20rebuild.20stage.201.20.2E.2E.2E
- https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F

Fixes #130980
Closes #131059

---

try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw
  • Loading branch information
bors committed Oct 8, 2024
2 parents 6f4ae0f + 5c01316 commit 18deb53
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 6 deletions.
6 changes: 6 additions & 0 deletions src/bootstrap/src/bin/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ fn main() {
cmd.args(lint_flags.split_whitespace());
}

// 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");
}

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,
Expand Down
15 changes: 13 additions & 2 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/rust-lang/rust/issues/131059> for details.
//
// Also see the discussion for properly handling I/O errors related to broken pipes, i.e. safe
// variants of `println!` in
// <https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F>.
cargo.rustflag("-Zon-broken-pipe=kill");

if builder.config.llvm_enzyme {
Expand Down
25 changes: 21 additions & 4 deletions src/bootstrap/src/core/build_steps/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
// <https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Applying.20.60-Zon-broken-pipe.3Dkill.60.20flags.20in.20bootstrap.3F>.
//
// For the rustc discussion, see
// <https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F>
// 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
Expand Down
73 changes: 73 additions & 0 deletions tests/run-make/broken-pipe-no-ice/rmake.rs
Original file line number Diff line number Diff line change
@@ -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 <https://github.com/rust-lang/rust/issues/34376>.

//@ 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);
}

0 comments on commit 18deb53

Please sign in to comment.