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

Tracking Issue for gracefully handling broken pipes in the compiler #131436

Open
8 tasks
jieyouxu opened this issue Oct 9, 2024 · 2 comments
Open
8 tasks

Tracking Issue for gracefully handling broken pipes in the compiler #131436

jieyouxu opened this issue Oct 9, 2024 · 2 comments
Labels
C-bug Category: This is a bug. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Oct 9, 2024

Context

std print! and println! by default will panic on a broken pipe if -Zon-broken-pipe=kill is not set when building rustc itself and left as default. If such a panic occurs and is not otherwise caught, it will manifest as an ICE. In bootstrap we build rustc with -Zon-broken-pipe=kill which terminates rustc to paper over issues like rustc --print=sysroot | false ICEing from the I/O panic from a broken pipe, but this is not always the desirabled behavior. As Nora said:

rustc --print=target-list | head -n5 should definitely work as expected and print only 5 targets and exit successfully, ICEing or erroring are not acceptable imo
so kill should still be passed
and rustc --print=target-list >/dev/full emitting an error instead of crashing would be neat too

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 a timeline of how we ended up with the -Zon-broken-pipe=kill paper.

Prior Art

Cargo denies print{,ln}! usages via clippy::print_std{err,out}:

See:

Steps

  • Survey current usages of print{,ln}! macro usages in rustc.
  • Classify desired behavior if we do handle I/O errors if we use some safe_print{,ln} alternative instead of panicking like print{,ln} (some might want to exit with success, some might want to error, but we probably never want to ICE).
  • Open an MCP to propose migrating print{,ln}! macro usages to properly handle errors and adding an internal lint to deny (in CI, but allow locally to still allow printf debugging) raw usages of print{,ln}!.
  • Fix existing print{,ln}! macro usages to properly handle errors.
  • Drop -Zon-broken-pipe=kill when building rustc.
  • Update tests/run-make/broken-pipe-no-ice/rmake.rs regression test.
  • Implement the internal lint.
  • Add documentation about print{,ln}! macro usages in the dev-guide.
@jieyouxu jieyouxu added C-bug Category: This is a bug. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2024
@jieyouxu jieyouxu added the E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. label Oct 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 9, 2024
Ignore broken-pipe-no-ice on apple (specifically macOS) for now

This test fails for me locally (initially reported by Zalathar) because apparently on macOS it doesn't say "internal compiler error" but it does report the std I/O panic, and it doesn't exit with a code of 101 but instead terminates with a wait signal of SIGPIPE.

Ignore this test on apple for now, until we try to actually address the underlying issue.

See rust-lang#131155 and rust-lang#131436 for more context.
jieyouxu added a commit to jieyouxu/rust that referenced this issue Oct 9, 2024
Ignore broken-pipe-no-ice on apple (specifically macOS) for now

This test fails for me locally (initially reported by Zalathar) because apparently on macOS it doesn't say "internal compiler error" but it does report the std I/O panic, and it doesn't exit with a code of 101 but instead terminates with a wait signal of SIGPIPE.

Ignore this test on apple for now, until we try to actually address the underlying issue.

See rust-lang#131155 and rust-lang#131436 for more context.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 9, 2024
Ignore broken-pipe-no-ice on apple (specifically macOS) for now

This test fails for me locally (initially reported by Zalathar) because apparently on macOS it doesn't say "internal compiler error" but it does report the std I/O panic, and it doesn't exit with a code of 101 but instead terminates with a wait signal of SIGPIPE.

Ignore this test on apple for now, until we try to actually address the underlying issue.

See rust-lang#131155 and rust-lang#131436 for more context.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 10, 2024
Rollup merge of rust-lang#131435 - jieyouxu:macos-pipe, r=Zalathar

Ignore broken-pipe-no-ice on apple (specifically macOS) for now

This test fails for me locally (initially reported by Zalathar) because apparently on macOS it doesn't say "internal compiler error" but it does report the std I/O panic, and it doesn't exit with a code of 101 but instead terminates with a wait signal of SIGPIPE.

Ignore this test on apple for now, until we try to actually address the underlying issue.

See rust-lang#131155 and rust-lang#131436 for more context.
@RalfJung
Copy link
Member

Isn't this more of a t-libs-api issue? Surely the Rust compiler is not the only project that has a need for a more graceful handling of broken pipes.

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 10, 2024

Well yes, but this is specifically about the rustc binary itself. For rustc itself, we may have different desired behaviors if we do encounter (different kinds of) I/O errors with print/println macro usages and not simply error/terminate. Other programs (not necessarily unix cli binaries) may have different requirements like servers and such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants