Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS` #131155

jieyouxu · 2024-10-02T14:02:23Z

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

rust/src/bootstrap/src/core/build_steps/compile.rs

Line 1058 in 1e5719b

cargo.rustflag("-Zon-broken-pipe=kill");

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

rust/src/bootstrap/src/core/build_steps/compile.rs

Line 1058 in 1e5719b

cargo.rustflag("-Zon-broken-pipe=kill");

, and the test failed because rustc ICE'd.

Manual testing

I have manually tried:

./x clean && ./x test build --stage 1->rustc +stage1 --print=sysroot | false`: no ICE.
./x clean -> ./x test run-make twice: no stage 1 cargo rebuilds.
./x clean -> ./x build rustdoc -> rustdoc +stage1 --version | false: no panics.
./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!

Fixes #130980
Closes #131059

try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw

jieyouxu · 2024-10-03T17:40:25Z

tests/run-make/broken-pipe-no-ice/rmake.rs

+
+fn main() {
+    let mut rustc = Command::new(env_var("RUSTC"));
+    rustc.arg("--print=sysroot");


Note: this is technically not robust because I exploited the fact that --print=sysroot and friends usually use the raw println! which will panic upon encountering a broken pipe when rustc is not built with -Zon-broken-pipe=kill. Some outputs in rustc_driver_impl are correctly guarded by using safe_println! where they won't panic upon encountering a broken pipe even if -Zon-broken-pipe=kill is unset.

but if -Zon-broken-pipe=kill is unset, safe_println will still emit an IO error, right? which is wrong

but if -Zon-broken-pipe=kill is unset, safe_println will still emit an IO error, right? which is wrong

Yeah idk how specifically that will be handled, but that is left as an exercise for the future reader. Actually I think to clarify, I believe the safe_println in rustc_driver_impl is to convert the panic if -Zon-broken-pipe=kill is unset -> fatal error, just that it avoids an ICE, not necessarily the behavior one would want...

tests/run-make/broken-pipe-no-ice/rmake.rs

jieyouxu · 2024-10-03T18:22:46Z

r? @onur-ozkan since you said you'd like to review this. Thanks for the help as well!

jieyouxu · 2024-10-03T18:34:10Z

cc @rust-lang/cargo since this PR should not affect how cargo is built (i.e. cargo should still not be built with -Zon-broken-pipe=kill, would appreciate a double-check.

cc @rust-lang/rustdoc because rustdoc is also built (both before this PR and after) with -Zon-broken-pipe=kill, and the regression test exercises the rustdoc binary and checks that it does not panic upon broken pipe. It is not clear to me if this is the desired behavior for rustdoc, hence this FYI.

ehuss · 2024-10-03T18:38:06Z

tests/run-make/broken-pipe-no-ice/rmake.rs

+    cmd.stdout(Stdio::piped()).stderr(Stdio::piped());
+
+    let mut child = cmd.spawn().unwrap();
+    // Close stdout
+    drop(child.stdout.take());


I would recommend caution with this approach, since it is inherently racey. For example, it assumes that it can close the handle before rustc is able to print. There can also be assumptions here about PIPE_BUF and other pipe behaviors. This can be unreliable on some systems. For example, on macOS, this should fail occasionally.

That's why cargo's tests which exercise this same behavior use a proc-macro to synchronize with rustc. (see here). That way the test can ensure that the handle is closed before rustc prints. Those tests also have some comments about the hazards of how this works.

Thanks for the context, I see why that test is so complex now...

Adjusted the test to use anonymous pipes instead which will make sure stdout is broken before the command is spawned.

GuillaumeGomez · 2024-10-03T19:22:37Z

because rustdoc is also built (both before this PR and after) with -Zon-broken-pipe=kill, and the regression test exercises the rustdoc binary and checks that it does not panic upon broken pipe. It is not clear to me if this is the desired behavior for rustdoc, hence this FYI.

I have to admit I have no idea. Being aligned with rustc seems the best course of action here.

src/bootstrap/src/bin/rustc.rs

src/bootstrap/src/core/build_steps/tool.rs

jieyouxu · 2024-10-05T12:49:24Z

Changes since last review:

Renamed untracked env var from UNTRACKED_BROKEN_PIPE_FLAG -> FORCE_ON_BROKEN_PIPE_KILL as suggested, and check for env var being set then in turn set -Zon-broken-pipe=kill explicitly in rustc bin shim to be clearer.
Improved clarity and locality of the comment for the untracked env var workaround.

Not yet ready for another round of reviews, I still need to fix the test.

src/bootstrap/src/bin/rustc.rs

- 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.

jieyouxu · 2024-10-08T02:24:57Z

@bors rollup=never (subtle build system modifications and a run-make test that has platform-specific behaviors)

jieyouxu · 2024-10-08T02:25:17Z

Updated the rustc bin shim comment and fixed the regression test so this should now be ready for review.

I ran the test locally on both:

aarch64-unknown-linux-gnu and
x86_64-pc-windows-msvc natively.

@rustbot ready

ChrisDenton · 2024-10-08T11:53:22Z

tests/run-make/broken-pipe-no-ice/rmake.rs

+            // 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.


You could try changing the windows hack to a early_fatal error:

rust/compiler/rustc_driver_impl/src/lib.rs

Line 1412 in cf24c73

let _ = early_dcx.early_err(msg.clone());

I think that would result in the exit code being 1.

I'll do that in a follow-up, I would like to stop run-make cargo rebuilds lol.
EDIT: actually I don't want to make the hack even hackier, this should be addressed by fixing the underlying cause and not add paper to the paper.

onur-ozkan · 2024-10-08T15:10:02Z

@bors r+

bors · 2024-10-08T15:10:06Z

📌 Commit 5c01316 has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-10-08T23:25:50Z

⌛ Testing commit 5c01316 with merge 18deb53...

bors · 2024-10-09T01:55:36Z

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 18deb53 to master...

rust-timer · 2024-10-09T03:16:09Z

Finished benchmarking commit (18deb53): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 774.51s -> 774.814s (0.04%)
Artifact size: 329.61 MiB -> 329.56 MiB (-0.02%)

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.

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.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 2, 2024

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the always-kill branch from e103cd0 to fcdd36a Compare October 3, 2024 15:19

jieyouxu changed the title ~~[WIP] Apply -Zon-broken-pipe=kill on rustc/rustdoc bin wrappers only~~ Fix tool build cache invalidation due to conditionally applied -Zon-broken-pipe=kill via tracked RUSTFLAGS Oct 3, 2024

jieyouxu changed the title ~~Fix tool build cache invalidation due to conditionally applied -Zon-broken-pipe=kill via tracked RUSTFLAGS~~ Stop building cargo from invalidating build cache of other tools due to conditionally applied -Zon-broken-pipe=kill via tracked RUSTFLAGS Oct 3, 2024

jieyouxu force-pushed the always-kill branch from fcdd36a to 2771ade Compare October 3, 2024 17:17

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Oct 3, 2024

jieyouxu commented Oct 3, 2024

View reviewed changes

tests/run-make/broken-pipe-no-ice/rmake.rs Show resolved Hide resolved

jieyouxu force-pushed the always-kill branch from 2771ade to 15de3dc Compare October 3, 2024 18:20

rustbot assigned onur-ozkan Oct 3, 2024

jieyouxu marked this pull request as ready for review October 3, 2024 18:22

jieyouxu added T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. and removed T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. labels Oct 3, 2024

ehuss reviewed Oct 3, 2024

View reviewed changes

onur-ozkan reviewed Oct 3, 2024

View reviewed changes

src/bootstrap/src/bin/rustc.rs Outdated Show resolved Hide resolved

src/bootstrap/src/core/build_steps/tool.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the always-kill branch from 15de3dc to 384c0c1 Compare October 5, 2024 12:45

onur-ozkan reviewed Oct 5, 2024

View reviewed changes

src/bootstrap/src/bin/rustc.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the always-kill branch 2 times, most recently from ea62b8e to b20436b Compare October 7, 2024 15:59

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

Use untracked env var to pass -Zon-broken-pipe=kill for tools

03abf67

- 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.

jieyouxu force-pushed the always-kill branch from b20436b to a45d0d9 Compare October 8, 2024 00:07

Add regression test for rustc/rustdoc broken pipe ICEs

5c01316

jieyouxu force-pushed the always-kill branch from a45d0d9 to 5c01316 Compare October 8, 2024 02:12

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2024

ChrisDenton reviewed Oct 8, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2024

jieyouxu mentioned this pull request Oct 9, 2024

[beta] backports #131423

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 9, 2024

bors merged commit 18deb53 into rust-lang:master Oct 9, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 9, 2024

jieyouxu deleted the always-kill branch October 9, 2024 01:57

Zalathar mentioned this pull request Oct 9, 2024

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

Merged

jieyouxu mentioned this pull request Oct 12, 2024

cargo is unnecessarily rebuilt on x.py dist #130018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS` #131155

Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS` #131155

jieyouxu commented Oct 2, 2024 •

edited

Loading

This comment has been minimized.

jieyouxu Oct 3, 2024

Noratrieb Oct 3, 2024

jieyouxu Oct 3, 2024 •

edited

Loading

jieyouxu commented Oct 3, 2024

jieyouxu commented Oct 3, 2024 •

edited

Loading

ehuss Oct 3, 2024

jieyouxu Oct 3, 2024

jieyouxu Oct 7, 2024

GuillaumeGomez commented Oct 3, 2024

jieyouxu commented Oct 5, 2024 •

edited

Loading

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

jieyouxu commented Oct 8, 2024

jieyouxu commented Oct 8, 2024 •

edited

Loading

ChrisDenton Oct 8, 2024

jieyouxu Oct 8, 2024 •

edited

Loading

ChrisDenton Oct 8, 2024

onur-ozkan commented Oct 8, 2024

bors commented Oct 8, 2024

bors commented Oct 8, 2024

bors commented Oct 9, 2024

rust-timer commented Oct 9, 2024

		// 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.

Prevent building cargo from invalidating build cache of other tools due to conditionally applied -Zon-broken-pipe=kill via tracked RUSTFLAGS #131155

Prevent building cargo from invalidating build cache of other tools due to conditionally applied -Zon-broken-pipe=kill via tracked RUSTFLAGS #131155

Conversation

jieyouxu commented Oct 2, 2024 • edited Loading

Approach

Testing

Integration testing

Manual testing

Related discussions

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Oct 3, 2024

jieyouxu commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Oct 3, 2024

jieyouxu commented Oct 5, 2024 • edited Loading

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

jieyouxu commented Oct 8, 2024

jieyouxu commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Oct 8, 2024

bors commented Oct 8, 2024

bors commented Oct 8, 2024

bors commented Oct 9, 2024

rust-timer commented Oct 9, 2024

Overall result: no relevant changes - no action needed

Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS` #131155

Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS` #131155

jieyouxu commented Oct 2, 2024 •

edited

Loading

jieyouxu Oct 3, 2024 •

edited

Loading

jieyouxu commented Oct 3, 2024 •

edited

Loading

jieyouxu commented Oct 5, 2024 •

edited

Loading

jieyouxu commented Oct 8, 2024 •

edited

Loading

jieyouxu Oct 8, 2024 •

edited

Loading