Auto merge of rust-lang#135461 - jieyouxu:migrate-jobserver-errors, r…

…=<try>

tests: Port `jobserver-error.rs` to rmake.rs

Part of rust-lang#121876.

This PR ports `jobserver-error.rs` to rmake.rs, and is basically rust-lang#128789 slightly adjusted. Namely, `set_aux_fd` is made `unsafe`, alongside some doc updates.

The complexity involved here is mostly how to get `/dev/null/` piping to fd 3 working with std `Command`, whereas with a shell this is much easier (as is evident with the `Makefile` version).

Supersedes rust-lang#128789.
This PR is co-authored with `@Oneirical` and `@coolreader18.`

r? `@ghost`

try-job: aarch64-gnu
try-job: i686-gnu-1
try-job: x86_64-gnu-debug
try-job: x86_64-gnu-llvm-18-1

src/tools/run-make-support/src/command.rs

@@ -151,6 +151,53 @@ impl Command {
         self
     }
 
+    /// Set an auxiliary stream passed to the process, besides the stdio streams.
+    ///
+    /// # Safety
+    ///
+    /// 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.
+    #[cfg(unix)]
+    pub unsafe fn set_aux_fd<F: Into<std::os::fd::OwnedFd>>(
+        &mut self,
+        new_fd: std::os::fd::RawFd,
+        fd: F,
+    ) -> &mut Self {
+        // 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(()) };
+
+        let fd = fd.into();
+        if fd.as_raw_fd() == new_fd {
+            // If the new file descriptor is already the same as fd, just turn off `FD_CLOEXEC`.
+
+            // SAFETY(io-safety): `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() != new_fd {
+                // SAFETY(io-safety): `new_fd` is not necessarily an unused fd. However, we're
+                // ensuring that `new_fd` 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 `new_fd` with the one
+                // from `fd`, allowing for shared access to the same underlying file or resource.
+                cvt(unsafe { libc::dup2(fd.as_raw_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()`].

src/tools/run-make-support/src/macros.rs

@@ -104,6 +104,23 @@ macro_rules! impl_common_helpers {
                 self
             }
 
+            /// Set an auxiliary stream passed to the process, besides the stdio streams.
+            ///
+            /// # Safety
+            ///
+            /// 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.
+            #[cfg(unix)]
+            pub unsafe fn set_aux_fd<F: Into<std::os::fd::OwnedFd>>(
+                &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 {

src/tools/tidy/src/allowed_run_make_makefiles.txt

@@ -1,6 +1,5 @@
 run-make/cat-and-grep-sanity-check/Makefile
 run-make/extern-fn-reachable/Makefile
-run-make/jobserver-error/Makefile
 run-make/split-debuginfo/Makefile
 run-make/symbol-mangling-hashed/Makefile
 run-make/translation/Makefile

tests/run-make/jobserver-error/rmake.rs

@@ -0,0 +1,55 @@
+//! 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
+//! </~https://github.com/rust-lang/rust/issues/46981>.
+
+//@ only-linux
+//@ ignore-cross-compile
+
+use run_make_support::{diff, rustc};
+
+fn main() {
+    let out = rustc()
+        .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();
+
+    // SAFETY(io-safety): we don't have overlapping fd 3.
+    let out = unsafe {
+        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): this case is spurious because:
+    //
+    // > 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
+    // </~https://github.com/rust-lang/rust/issues/110321#issuecomment-1636914956>.
+
+    //let (readpipe, _) = std::pipe::pipe().unwrap();
+    //let out = unsafe {
+    //    rustc()
+    //        .stdin("fn main() {}")
+    //        .input("-")
+    //        .env("MAKEFLAGS", "--jobserver-auth=3,3")
+    //        .set_aux_fd(3, readpipe)
+    //        .run()
+    //        .stderr_utf8()
+    //};
+    //diff().expected_file("poisoned_pipe.stderr").actual_text("actual", out).run();
+}