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

tests: Port jobserver-error to rmake.rs #135461

Merged
merged 2 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions src/tools/run-make-support/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved
/// 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<F: Into<std::os::fd::OwnedFd>>(
&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()`].
Expand Down
11 changes: 11 additions & 0 deletions src/tools/run-make-support/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<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 {
Expand Down
1 change: 0 additions & 1 deletion src/tools/tidy/src/allowed_run_make_makefiles.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
run-make/jobserver-error/Makefile
run-make/split-debuginfo/Makefile
run-make/symbol-mangling-hashed/Makefile
run-make/translation/Makefile
17 changes: 0 additions & 17 deletions tests/run-make/jobserver-error/Makefile

This file was deleted.

2 changes: 1 addition & 1 deletion tests/run-make/jobserver-error/cannot_open_fd.stderr
Original file line number Diff line number Diff line change
@@ -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

Expand Down
47 changes: 47 additions & 0 deletions tests/run-make/jobserver-error/rmake.rs
Original file line number Diff line number Diff line change
@@ -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
//! <https://github.com/rust-lang/rust/issues/46981>.
//@ 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
// <https://github.com/rust-lang/rust/issues/110321#issuecomment-1636914956>.
}
Loading