Auto merge of #113730 - belovdv:jobserver-init-check, r=petrochenkov

Report errors in jobserver inherited through environment variables

This pr attempts to catch situations, when jobserver exists, but is not being inherited.

r? `@petrochenkov`

Cargo.lock

@@ -2086,9 +2086,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",
 ]

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" }

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"

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
-
-// 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<Client> = 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 jobserver_crate::{FromEnv, FromEnvErrorKind};
+
+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<Result<Client, String>> = 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<Client> = 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();
 }

compiler/rustc_session/src/session.rs

@@ -24,7 +24,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;
@@ -1469,6 +1469,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,
@@ -1776,6 +1781,16 @@ impl EarlyErrorHandler {
     pub fn early_warn(&self, msg: impl Into<DiagnosticMessage>) {
         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<DiagnosticMessage>,
+        note: impl Into<SubdiagnosticMessage>,
+    ) {
+        self.handler.struct_warn(msg).note(note).emit()
+    }
 }
 
 fn mk_emitter(output: ErrorOutputType) -> Box<DynEmitter> {

src/tools/miri/cargo-miri/src/phases.rs

@@ -501,6 +501,14 @@ pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: Runner
     // Set missing env vars. We prefer build-time env vars over run-time ones; see
     // <https://github.com/rust-lang/miri/issues/1661> 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.

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</dev/null' 2>&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</dev/null' 2>&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 -
+

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
+

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
+