From 45e634234690a7d061e133b8cf9d31af34b29a0b Mon Sep 17 00:00:00 2001 From: belovdv <70999565+belovdv@users.noreply.github.com> Date: Sat, 15 Jul 2023 18:48:09 +0300 Subject: [PATCH] jobserver: check file descriptors --- Cargo.lock | 4 +- compiler/rustc_codegen_ssa/Cargo.toml | 2 +- compiler/rustc_data_structures/Cargo.toml | 2 +- .../rustc_data_structures/src/jobserver.rs | 92 +++++++++++++------ compiler/rustc_session/src/session.rs | 17 +++- src/tools/miri/cargo-miri/src/phases.rs | 8 ++ tests/run-make/jobserver-error/Makefile | 12 ++- .../jobserver-error/cannot_open_fd.stderr | 6 ++ .../jobserver-error/not_a_pipe.stderr | 4 + ...{jobserver.stderr => poisoned_pipe.stderr} | 0 10 files changed, 112 insertions(+), 35 deletions(-) create mode 100644 tests/run-make/jobserver-error/cannot_open_fd.stderr create mode 100644 tests/run-make/jobserver-error/not_a_pipe.stderr rename tests/run-make/jobserver-error/{jobserver.stderr => poisoned_pipe.stderr} (100%) diff --git a/Cargo.lock b/Cargo.lock index 1296468b4e2..bbcc9f57b56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2085,9 +2085,9 @@ dependencies = [ [[package]] name = "jobserver" -version = "0.1.26" +version = "0.1.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "936cfd212a0155903bcbc060e316fb6cc7cbf2e1907329391ebadc1fe0ce77c2" +checksum = "8c37f63953c4c63420ed5fd3d6d398c719489b9f872b9fa683262f8edd363c7d" dependencies = [ "libc", ] diff --git a/compiler/rustc_codegen_ssa/Cargo.toml b/compiler/rustc_codegen_ssa/Cargo.toml index 0ca19e5fe4a..3f2ed257d08 100644 --- a/compiler/rustc_codegen_ssa/Cargo.toml +++ b/compiler/rustc_codegen_ssa/Cargo.toml @@ -9,7 +9,7 @@ ar_archive_writer = "0.1.5" bitflags = "1.2.1" cc = "1.0.69" itertools = "0.11" -jobserver = "0.1.22" +jobserver = "0.1.27" pathdiff = "0.2.0" regex = "1.4" rustc_arena = { path = "../rustc_arena" } diff --git a/compiler/rustc_data_structures/Cargo.toml b/compiler/rustc_data_structures/Cargo.toml index 77d9e491748..4732783a12d 100644 --- a/compiler/rustc_data_structures/Cargo.toml +++ b/compiler/rustc_data_structures/Cargo.toml @@ -11,7 +11,7 @@ elsa = "=1.7.1" ena = "0.14.2" indexmap = { version = "2.0.0" } itertools = "0.11" -jobserver_crate = { version = "0.1.13", package = "jobserver" } +jobserver_crate = { version = "0.1.27", package = "jobserver" } libc = "0.2" measureme = "10.0.0" rustc-hash = "1.1.0" diff --git a/compiler/rustc_data_structures/src/jobserver.rs b/compiler/rustc_data_structures/src/jobserver.rs index 09baa3095a4..b777bfd4d3c 100644 --- a/compiler/rustc_data_structures/src/jobserver.rs +++ b/compiler/rustc_data_structures/src/jobserver.rs @@ -1,40 +1,78 @@ pub use jobserver_crate::Client; -use std::sync::LazyLock; -// We can only call `from_env` once per process +use jobserver_crate::{FromEnv, FromEnvErrorKind}; -// Note that this is unsafe because it may misinterpret file descriptors -// on Unix as jobserver file descriptors. We hopefully execute this near -// the beginning of the process though to ensure we don't get false -// positives, or in other words we try to execute this before we open -// any file descriptors ourselves. -// -// Pick a "reasonable maximum" if we don't otherwise have -// a jobserver in our environment, capping out at 32 so we -// don't take everything down by hogging the process run queue. -// The fixed number is used to have deterministic compilation -// across machines. -// -// Also note that we stick this in a global because there could be -// multiple rustc instances in this process, and the jobserver is -// per-process. -static GLOBAL_CLIENT: LazyLock = LazyLock::new(|| unsafe { - Client::from_env().unwrap_or_else(|| { - let client = Client::new(32).expect("failed to create jobserver"); - // Acquire a token for the main thread which we can release later - client.acquire_raw().ok(); - client - }) +use std::sync::{LazyLock, OnceLock}; + +// We can only call `from_env_ext` once per process + +// We stick this in a global because there could be multiple rustc instances +// in this process, and the jobserver is per-process. +static GLOBAL_CLIENT: LazyLock> = LazyLock::new(|| { + // Note that this is unsafe because it may misinterpret file descriptors + // on Unix as jobserver file descriptors. We hopefully execute this near + // the beginning of the process though to ensure we don't get false + // positives, or in other words we try to execute this before we open + // any file descriptors ourselves. + let FromEnv { client, var } = unsafe { Client::from_env_ext(true) }; + + let error = match client { + Ok(client) => return Ok(client), + Err(e) => e, + }; + + if matches!( + error.kind(), + FromEnvErrorKind::NoEnvVar | FromEnvErrorKind::NoJobserver | FromEnvErrorKind::Unsupported + ) { + return Ok(default_client()); + } + + // Environment specifies jobserver, but it looks incorrect. + // Safety: `error.kind()` should be `NoEnvVar` if `var == None`. + let (name, value) = var.unwrap(); + Err(format!( + "failed to connect to jobserver from environment variable `{name}={:?}`: {error}", + value + )) }); +// Create a new jobserver if there's no inherited one. +fn default_client() -> Client { + // Pick a "reasonable maximum" capping out at 32 + // so we don't take everything down by hogging the process run queue. + // The fixed number is used to have deterministic compilation across machines. + let client = Client::new(32).expect("failed to create jobserver"); + + // Acquire a token for the main thread which we can release later + client.acquire_raw().ok(); + + client +} + +static GLOBAL_CLIENT_CHECKED: OnceLock = OnceLock::new(); + +pub fn check(report_warning: impl FnOnce(&'static str)) { + let client_checked = match &*GLOBAL_CLIENT { + Ok(client) => client.clone(), + Err(e) => { + report_warning(e); + default_client() + } + }; + GLOBAL_CLIENT_CHECKED.set(client_checked).ok(); +} + +const ACCESS_ERROR: &str = "jobserver check should have been called earlier"; + pub fn client() -> Client { - GLOBAL_CLIENT.clone() + GLOBAL_CLIENT_CHECKED.get().expect(ACCESS_ERROR).clone() } pub fn acquire_thread() { - GLOBAL_CLIENT.acquire_raw().ok(); + GLOBAL_CLIENT_CHECKED.get().expect(ACCESS_ERROR).acquire_raw().ok(); } pub fn release_thread() { - GLOBAL_CLIENT.release_raw().ok(); + GLOBAL_CLIENT_CHECKED.get().expect(ACCESS_ERROR).release_raw().ok(); } diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 20a67d6d036..515b412315f 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -26,7 +26,7 @@ use rustc_errors::registry::Registry; use rustc_errors::{ error_code, fallback_fluent_bundle, DiagnosticBuilder, DiagnosticId, DiagnosticMessage, ErrorGuaranteed, FluentBundle, Handler, IntoDiagnostic, LazyFallbackBundle, MultiSpan, Noted, - TerminalUrl, + SubdiagnosticMessage, TerminalUrl, }; use rustc_macros::HashStable_Generic; pub use rustc_span::def_id::StableCrateId; @@ -1479,6 +1479,11 @@ pub fn build_session( let asm_arch = if target_cfg.allow_asm { InlineAsmArch::from_str(&target_cfg.arch).ok() } else { None }; + // Check jobserver before getting `jobserver::client`. + jobserver::check(|err| { + handler.early_warn_with_note(err, "the build environment is likely misconfigured") + }); + let sess = Session { target: target_cfg, host, @@ -1762,6 +1767,16 @@ impl EarlyErrorHandler { pub fn early_warn(&self, msg: impl Into) { self.handler.struct_warn(msg).emit() } + + #[allow(rustc::untranslatable_diagnostic)] + #[allow(rustc::diagnostic_outside_of_impl)] + pub fn early_warn_with_note( + &self, + msg: impl Into, + note: impl Into, + ) { + self.handler.struct_warn(msg).note(note).emit() + } } fn mk_emitter(output: ErrorOutputType) -> Box { diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index d655df6d994..a7412f90c85 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -501,6 +501,14 @@ pub fn phase_runner(mut binary_args: impl Iterator, phase: Runner // Set missing env vars. We prefer build-time env vars over run-time ones; see // for the kind of issue that fixes. for (name, val) in info.env { + // `CARGO_MAKEFLAGS` contains information about how to reach the + // jobserver, but by the time the program is being run, that jobserver + // no longer exists. Hence we shouldn't forward this. + // FIXME: Miri builds the final crate without a jobserver. + // This may be fixed with github.com/rust-lang/cargo/issues/12597. + if name == "CARGO_MAKEFLAGS" { + continue; + } if let Some(old_val) = env::var_os(&name) { if old_val == val { // This one did not actually change, no need to re-set it. diff --git a/tests/run-make/jobserver-error/Makefile b/tests/run-make/jobserver-error/Makefile index 4a1699cc740..a7601b86715 100644 --- a/tests/run-make/jobserver-error/Makefile +++ b/tests/run-make/jobserver-error/Makefile @@ -1,9 +1,15 @@ include ../tools.mk # only-linux -# ignore-test: This test randomly fails, see https://github.com/rust-lang/rust/issues/110321 +# ignore-cross-compile -# Test compiler behavior in case: `jobserver-auth` points to correct pipe which is not jobserver. +# Test compiler behavior in case environment specifies wrong jobserver. all: - bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=3,3" $(RUSTC) - 3&1 | diff jobserver.stderr - + bash -c 'echo "fn main() {}" | MAKEFLAGS="--jobserver-auth=3,3" $(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 new file mode 100644 index 00000000000..343de5cd52c --- /dev/null +++ b/tests/run-make/jobserver-error/cannot_open_fd.stderr @@ -0,0 +1,6 @@ +warning: failed to connect to jobserver from environment variable `MAKEFLAGS="--jobserver-auth=3,3"`: cannot open file descriptor 3 from the jobserver environment variable value: Bad file descriptor (os error 9) + | + = note: the build environment is likely misconfigured + +error: no input filename given + diff --git a/tests/run-make/jobserver-error/not_a_pipe.stderr b/tests/run-make/jobserver-error/not_a_pipe.stderr new file mode 100644 index 00000000000..536c04576b9 --- /dev/null +++ b/tests/run-make/jobserver-error/not_a_pipe.stderr @@ -0,0 +1,4 @@ +warning: failed to connect to jobserver from environment variable `MAKEFLAGS="--jobserver-auth=3,3"`: file descriptor 3 from the jobserver environment variable value is not a pipe + | + = note: the build environment is likely misconfigured + diff --git a/tests/run-make/jobserver-error/jobserver.stderr b/tests/run-make/jobserver-error/poisoned_pipe.stderr similarity index 100% rename from tests/run-make/jobserver-error/jobserver.stderr rename to tests/run-make/jobserver-error/poisoned_pipe.stderr