From ead78bb2435defbdf0f50d3affa9344ef11047d2 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?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 14 Jan 2025 07:03:54 +0800 Subject: [PATCH 1/2] run-make-support: add `set_aux_fd` helper Co-authored-by: Noa Co-authored-by: Oneirical --- src/tools/run-make-support/src/command.rs | 55 +++++++++++++++++++++++ src/tools/run-make-support/src/macros.rs | 11 +++++ 2 files changed, 66 insertions(+) diff --git a/src/tools/run-make-support/src/command.rs b/src/tools/run-make-support/src/command.rs index e73413085fade..b4dc753ab5347 100644 --- a/src/tools/run-make-support/src/command.rs +++ b/src/tools/run-make-support/src/command.rs @@ -151,6 +151,61 @@ impl Command { self } + /// Set an auxiliary stream passed to the process, besides the stdio streams. + /// + /// # Notes + /// + /// Use with caution! Ideally, only set one aux fd; if there are multiple, their old `fd` may + /// overlap with another's `new_fd`, and may break. The caller must make sure this is not the + /// case. This function is only "safe" because the safety requirements are practically not + /// possible to uphold. + #[cfg(unix)] + pub fn set_aux_fd>( + &mut self, + new_fd: std::os::fd::RawFd, + fd: F, + ) -> &mut Self { + use std::mem; + // NOTE: If more than 1 auxiliary file descriptor is needed, this function should be + // rewritten. + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt; + + let cvt = |x| if x == -1 { Err(std::io::Error::last_os_error()) } else { Ok(()) }; + + // Ensure fd stays open until the fork. + let fd = mem::ManuallyDrop::new(fd.into()); + let fd = fd.as_raw_fd(); + + if fd == new_fd { + // If the new file descriptor is already the same as fd, just turn off `FD_CLOEXEC`. + let fd_flags = { + let ret = unsafe { libc::fcntl(fd, libc::F_GETFD, 0) }; + if ret < 0 { + panic!("failed to read fd flags: {}", std::io::Error::last_os_error()); + } + ret + }; + // Clear `FD_CLOEXEC`. + let fd_flags = fd_flags & !libc::FD_CLOEXEC; + + // SAFETY(io-safety): `fd` is already owned. + cvt(unsafe { libc::fcntl(fd, libc::F_SETFD, fd_flags as libc::c_int) }) + .expect("disabling CLOEXEC failed"); + } + let pre_exec = move || { + if fd.as_raw_fd() != new_fd { + // SAFETY(io-safety): it's the caller's responsibility that we won't override the + // target fd. + cvt(unsafe { libc::dup2(fd, new_fd) })?; + } + Ok(()) + }; + // SAFETY(pre-exec-safe): `dup2` is pre-exec-safe. + unsafe { self.cmd.pre_exec(pre_exec) }; + self + } + /// Run the constructed command and assert that it is successfully run. /// /// By default, std{in,out,err} are [`Stdio::piped()`]. diff --git a/src/tools/run-make-support/src/macros.rs b/src/tools/run-make-support/src/macros.rs index cc3d1281d0ab2..94955aefe57aa 100644 --- a/src/tools/run-make-support/src/macros.rs +++ b/src/tools/run-make-support/src/macros.rs @@ -104,6 +104,17 @@ macro_rules! impl_common_helpers { self } + /// Set an auxiliary stream passed to the process, besides the stdio streams. + #[cfg(unix)] + pub fn set_aux_fd>( + &mut self, + new_fd: std::os::fd::RawFd, + fd: F, + ) -> &mut Self { + self.cmd.set_aux_fd(new_fd, fd); + self + } + /// Run the constructed command and assert that it is successfully run. #[track_caller] pub fn run(&mut self) -> crate::command::CompletedProcess { From 20229200c80c7a6400f455a2cc2281045c302ba6 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?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 14 Jan 2025 07:04:51 +0800 Subject: [PATCH 2/2] tests: port `jobserver-error.rs` to rmake.rs Co-authored-by: Noa Co-authored-by: Oneirical --- .../tidy/src/allowed_run_make_makefiles.txt | 1 - tests/run-make/jobserver-error/Makefile | 17 ------- .../jobserver-error/cannot_open_fd.stderr | 2 +- tests/run-make/jobserver-error/rmake.rs | 47 +++++++++++++++++++ 4 files changed, 48 insertions(+), 19 deletions(-) delete mode 100644 tests/run-make/jobserver-error/Makefile create mode 100644 tests/run-make/jobserver-error/rmake.rs diff --git a/src/tools/tidy/src/allowed_run_make_makefiles.txt b/src/tools/tidy/src/allowed_run_make_makefiles.txt index 567f0a256d98b..6f0fd09b353a5 100644 --- a/src/tools/tidy/src/allowed_run_make_makefiles.txt +++ b/src/tools/tidy/src/allowed_run_make_makefiles.txt @@ -1,4 +1,3 @@ -run-make/jobserver-error/Makefile run-make/split-debuginfo/Makefile run-make/symbol-mangling-hashed/Makefile run-make/translation/Makefile diff --git a/tests/run-make/jobserver-error/Makefile b/tests/run-make/jobserver-error/Makefile deleted file mode 100644 index 9f34970f96f10..0000000000000 --- a/tests/run-make/jobserver-error/Makefile +++ /dev/null @@ -1,17 +0,0 @@ -include ../tools.mk - -# only-linux -# ignore-cross-compile - -# Test compiler behavior in case environment specifies wrong jobserver. -# Note that by default, the compiler uses file descriptors 0 (stdin), 1 (stdout), 2 (stderr), -# but also 3 and 4 for either end of the ctrl-c signal handler self-pipe. - -all: - bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=5,5" $(RUSTC)' 2>&1 | diff cannot_open_fd.stderr - - bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=3,3" $(RUSTC) - 3&1 | diff not_a_pipe.stderr - - -# This test randomly fails, see https://github.com/rust-lang/rust/issues/110321 -disabled: - bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=3,3" $(RUSTC) - 3< <(cat /dev/null)' 2>&1 | diff poisoned_pipe.stderr - - diff --git a/tests/run-make/jobserver-error/cannot_open_fd.stderr b/tests/run-make/jobserver-error/cannot_open_fd.stderr index 9ac4c1c58f721..d075057b3d3df 100644 --- a/tests/run-make/jobserver-error/cannot_open_fd.stderr +++ b/tests/run-make/jobserver-error/cannot_open_fd.stderr @@ -1,4 +1,4 @@ -warning: failed to connect to jobserver from environment variable `MAKEFLAGS="--jobserver-auth=5,5"`: cannot open file descriptor 5 from the jobserver environment variable value: Bad file descriptor (os error 9) +warning: failed to connect to jobserver from environment variable `MAKEFLAGS="--jobserver-auth=1000,1000"`: cannot open file descriptor 1000 from the jobserver environment variable value: Bad file descriptor (os error 9) | = note: the build environment is likely misconfigured diff --git a/tests/run-make/jobserver-error/rmake.rs b/tests/run-make/jobserver-error/rmake.rs new file mode 100644 index 0000000000000..14ee24c714882 --- /dev/null +++ b/tests/run-make/jobserver-error/rmake.rs @@ -0,0 +1,47 @@ +// ignore-tidy-linelength +//! If the environment variables contain an invalid `jobserver-auth`, this used to cause an ICE +//! until this was fixed in [do not panic on failure to acquire jobserver token +//! #109694](https://github.com/rust-lang/rust/pull/109694). +//! +//! Proper handling has been added, and this test checks that helpful warnings and errors are +//! printed instead in case of a wrong jobserver. See +//! . + +//@ only-linux +//@ ignore-cross-compile + +#![deny(warnings)] + +use run_make_support::{diff, rustc}; + +fn main() { + let out = rustc() + .stdin_buf(("fn main() {}").as_bytes()) + .env("MAKEFLAGS", "--jobserver-auth=1000,1000") + .run_fail() + .stderr_utf8(); + diff().expected_file("cannot_open_fd.stderr").actual_text("actual", out).run(); + + let out = rustc() + .stdin_buf(("fn main() {}").as_bytes()) + .input("-") + .env("MAKEFLAGS", "--jobserver-auth=3,3") + .set_aux_fd(3, std::fs::File::open("/dev/null").unwrap()) + .run() + .stderr_utf8(); + diff().expected_file("not_a_pipe.stderr").actual_text("actual", out).run(); + + // FIXME(#110321): the Makefile version had a disabled check: + // + // ```makefile + // bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=3,3" $(RUSTC) - 3< <(cat /dev/null)' 2>&1 | diff poisoned_pipe.stderr - + // ``` + // + // > the jobserver helper thread launched here gets starved out and doesn't run, while the + // > coordinator thread continually processes work using the implicit jobserver token, never + // > yielding long enough for the jobserver helper to do its work (and process the error). + // + // but is not necessarily worth fixing as it might require changing coordinator behavior that + // might regress performance. See discussion at + // . +}