From edd750e4b58518040b74e138953223f5a86e9ce3 Mon Sep 17 00:00:00 2001 From: Oneirical Date: Wed, 7 Aug 2024 11:56:10 -0400 Subject: [PATCH 1/3] rewrite jobserver-error to rmake --- .../tidy/src/allowed_run_make_makefiles.txt | 1 - tests/run-make/jobserver-error/Makefile | 17 -------- tests/run-make/jobserver-error/rmake.rs | 40 +++++++++++++++++++ 3 files changed, 40 insertions(+), 18 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 50d21c7ed4c99..6534f42b965f5 100644 --- a/src/tools/tidy/src/allowed_run_make_makefiles.txt +++ b/src/tools/tidy/src/allowed_run_make_makefiles.txt @@ -4,7 +4,6 @@ run-make/emit-to-stdout/Makefile run-make/extern-fn-reachable/Makefile run-make/incr-add-rust-src-component/Makefile run-make/issue-84395-lto-embed-bitcode/Makefile -run-make/jobserver-error/Makefile run-make/libs-through-symlinks/Makefile run-make/split-debuginfo/Makefile run-make/symbol-mangling-hashed/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/rmake.rs b/tests/run-make/jobserver-error/rmake.rs new file mode 100644 index 0000000000000..a17add2d81fb8 --- /dev/null +++ b/tests/run-make/jobserver-error/rmake.rs @@ -0,0 +1,40 @@ +// If the environment variables contain an invalid `jobserver-auth`, this used +// to cause an ICE (internal compiler error) until this was fixed in #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 https://github.com/rust-lang/rust/issues/46981 + +// FIXME(Oneirical): The original test included this memo: +// # 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. + +// FIXME(Oneirical): only-linux ignore-cross-compile + +use run_make_support::{diff, rfs, rustc}; + +fn main() { + let out = rustc() + .stdin("fn main() {}") + .env("MAKEFLAGS", "--jobserver-auth=5,5") + .run_fail() + .stderr_utf8(); + diff().expected_file("cannot_open_fd.stderr").actual_text("actual", out).run(); + // FIXME(Oneirical): Find how to use file descriptor "3" with run-make-support + // and pipe /dev/null into it. + // Original lines: + // + // 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 - + // + // let out = rustc() + // .stdin("fn main() {}") + // .input("-") + // .env("MAKEFLAGS", "--jobserver-auth=0,0") + // .run() + // .stderr_utf8(); + // diff().expected_file("not_a_pipe.stderr").actual_text("actual", out).run(); +} From f6f9e106e804c0b274fb12bcb37d15d25afd78fa Mon Sep 17 00:00:00 2001 From: Noa Date: Thu, 12 Sep 2024 16:21:54 -0400 Subject: [PATCH 2/3] Add fd3 support to runmake --- src/tools/run-make-support/Cargo.toml | 1 + src/tools/run-make-support/src/command.rs | 25 ++++++++++++++++++++++ src/tools/run-make-support/src/macros.rs | 11 ++++++++++ tests/run-make/jobserver-error/rmake.rs | 26 ++++++++++++----------- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/tools/run-make-support/Cargo.toml b/src/tools/run-make-support/Cargo.toml index d3605cd3dce05..105b3e2ed2239 100644 --- a/src/tools/run-make-support/Cargo.toml +++ b/src/tools/run-make-support/Cargo.toml @@ -10,6 +10,7 @@ similar = "2.5.0" wasmparser = { version = "0.216", default-features = false, features = ["std"] } regex = "1.8" # 1.8 to avoid memchr 2.6.0, as 2.5.0 is pinned in the workspace gimli = "0.31.0" +libc = "0.2" build_helper = { path = "../build_helper" } serde_json = "1.0" libc = "0.2" diff --git a/src/tools/run-make-support/src/command.rs b/src/tools/run-make-support/src/command.rs index 6b58173b34304..ee900d3cac484 100644 --- a/src/tools/run-make-support/src/command.rs +++ b/src/tools/run-make-support/src/command.rs @@ -151,6 +151,31 @@ impl Command { self } + /// Set an auxiliary stream passed to the process, besides the stdio streams. + #[cfg(unix)] + pub fn set_aux_fd>( + &mut self, + newfd: std::os::fd::RawFd, + fd: F, + ) -> &mut Self { + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt; + + let fd = fd.into(); + unsafe { + self.cmd.pre_exec(move || { + let fd = fd.as_raw_fd(); + let ret = if fd == newfd { + libc::fcntl(fd, libc::F_SETFD, 0) + } else { + libc::dup2(fd, newfd) + }; + if ret == -1 { Err(std::io::Error::last_os_error()) } else { Ok(()) } + }); + } + 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 f7fe4f5422399..322ee5314e239 100644 --- a/src/tools/run-make-support/src/macros.rs +++ b/src/tools/run-make-support/src/macros.rs @@ -80,6 +80,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, + newfd: std::os::fd::RawFd, + fd: F, + ) -> &mut Self { + self.cmd.set_aux_fd(newfd, fd); + self + } + /// Run the constructed command and assert that it is successfully run. #[track_caller] pub fn run(&mut self) -> crate::command::CompletedProcess { diff --git a/tests/run-make/jobserver-error/rmake.rs b/tests/run-make/jobserver-error/rmake.rs index a17add2d81fb8..c18c226285740 100644 --- a/tests/run-make/jobserver-error/rmake.rs +++ b/tests/run-make/jobserver-error/rmake.rs @@ -19,22 +19,24 @@ fn main() { .run_fail() .stderr_utf8(); diff().expected_file("cannot_open_fd.stderr").actual_text("actual", out).run(); - // FIXME(Oneirical): Find how to use file descriptor "3" with run-make-support - // and pipe /dev/null into it. - // Original lines: - // - // bash -c 'echo "fn main() {}" | makeflags="--jobserver-auth=3,3" $(rustc) - 3&1 | diff not_a_pipe.stderr - + + let out = rustc() + .stdin("fn main() {}") + .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(); + // # 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 - - // + // let (readpipe, _) = std::pipe::pipe().unwrap(); // let out = rustc() // .stdin("fn main() {}") // .input("-") - // .env("MAKEFLAGS", "--jobserver-auth=0,0") + // .env("MAKEFLAGS", "--jobserver-auth=3,3") + // .set_fd3(readpipe) // .run() // .stderr_utf8(); - // diff().expected_file("not_a_pipe.stderr").actual_text("actual", out).run(); + // diff().expected_file("poisoned_pipe.stderr").actual_text("actual", out).run(); } From 482fa51f410d41ceb3688390af4f014a305208bb Mon Sep 17 00:00:00 2001 From: Noa Date: Fri, 13 Sep 2024 11:51:37 -0400 Subject: [PATCH 3/3] Better comments for set_aux_fd --- src/tools/run-make-support/Cargo.toml | 1 - src/tools/run-make-support/src/command.rs | 37 +++++++++++++++++------ src/tools/run-make-support/src/macros.rs | 4 +++ tests/run-make/jobserver-error/rmake.rs | 10 ++---- 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/tools/run-make-support/Cargo.toml b/src/tools/run-make-support/Cargo.toml index 105b3e2ed2239..cdc49d45c3d61 100644 --- a/src/tools/run-make-support/Cargo.toml +++ b/src/tools/run-make-support/Cargo.toml @@ -13,4 +13,3 @@ gimli = "0.31.0" libc = "0.2" build_helper = { path = "../build_helper" } serde_json = "1.0" -libc = "0.2" diff --git a/src/tools/run-make-support/src/command.rs b/src/tools/run-make-support/src/command.rs index ee900d3cac484..7d3e4ca845cd8 100644 --- a/src/tools/run-make-support/src/command.rs +++ b/src/tools/run-make-support/src/command.rs @@ -152,6 +152,10 @@ impl Command { } /// Set an auxiliary stream passed to the process, besides the stdio streams. + /// Use with caution - ideally, only set one aux fd; if there are multiple, + /// their old `fd` may overlap with another's `newfd`, and may break. + //FIXME: If more than 1 auxiliary file descriptor is needed, this function + // should be rewritten. #[cfg(unix)] pub fn set_aux_fd>( &mut self, @@ -161,18 +165,31 @@ impl Command { 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(()) }; + let fd = fd.into(); - unsafe { - self.cmd.pre_exec(move || { - let fd = fd.as_raw_fd(); - let ret = if fd == newfd { - libc::fcntl(fd, libc::F_SETFD, 0) - } else { - libc::dup2(fd, newfd) - }; - if ret == -1 { Err(std::io::Error::last_os_error()) } else { Ok(()) } - }); + if fd.as_raw_fd() == newfd { + // if the new file descriptor is already the same as fd, just turn off FD_CLOEXEC + // SAFETY: io-safe: fd is already owned. + cvt(unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_SETFD, 0) }) + .expect("disabling CLOEXEC failed"); + // The pre_exec function should be unconditionally set, since it captures + // `fd`, and this ensures that it stays open until the fork } + let pre_exec = move || { + if fd.as_raw_fd() != newfd { + // SAFETY: io-"safe": newfd is not necessarily an unused fd. + // However, we're ensuring that newfd will now refer to the same file descriptor + // as fd, which is safe as long as we manage the lifecycle of both descriptors + // correctly. This operation will replace the file descriptor referred to by newfd + // with the one from fd, allowing for shared access to the same underlying file or + // resource. + cvt(unsafe { libc::dup2(fd.as_raw_fd(), newfd) })?; + } + Ok(()) + }; + // SAFETY: dup2 is pre-exec-safe + unsafe { self.cmd.pre_exec(pre_exec) }; self } diff --git a/src/tools/run-make-support/src/macros.rs b/src/tools/run-make-support/src/macros.rs index 322ee5314e239..b2a14d8f881f8 100644 --- a/src/tools/run-make-support/src/macros.rs +++ b/src/tools/run-make-support/src/macros.rs @@ -81,6 +81,10 @@ macro_rules! impl_common_helpers { } /// Set an auxiliary stream passed to the process, besides the stdio streams. + /// Use with caution - ideally, only set one aux fd; if there are multiple, + /// their old `fd` may overlap with another's `newfd`, and may break. + //FIXME: If more than 1 auxiliary file descriptor is needed, this function + // should be rewritten. #[cfg(unix)] pub fn set_aux_fd>( &mut self, diff --git a/tests/run-make/jobserver-error/rmake.rs b/tests/run-make/jobserver-error/rmake.rs index c18c226285740..e99a94e4ce514 100644 --- a/tests/run-make/jobserver-error/rmake.rs +++ b/tests/run-make/jobserver-error/rmake.rs @@ -4,24 +4,20 @@ // and errors are printed instead in case of a wrong jobserver. // See https://github.com/rust-lang/rust/issues/46981 -// FIXME(Oneirical): The original test included this memo: -// # 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. - // FIXME(Oneirical): only-linux ignore-cross-compile -use run_make_support::{diff, rfs, rustc}; +use run_make_support::{diff, rustc}; fn main() { let out = rustc() - .stdin("fn main() {}") + .stdin_buf(("fn main() {}").as_bytes()) .env("MAKEFLAGS", "--jobserver-auth=5,5") .run_fail() .stderr_utf8(); diff().expected_file("cannot_open_fd.stderr").actual_text("actual", out).run(); let out = rustc() - .stdin("fn main() {}") + .stdin_buf(("fn main() {}").as_bytes()) .input("-") .env("MAKEFLAGS", "--jobserver-auth=3,3") .set_aux_fd(3, std::fs::File::open("/dev/null").unwrap())