Auto merge of rust-lang#2552 - RalfJung:env-term, r=oli-obk

remove Windows TERM env var hack and -Zmiri-env-exclude

The hack should not be needed any more since rust-lang#100206. And that also mostly removes the need for `-Zmiri-env-exclude` -- if needed, users can still achieve the same by running `(unset VAR; cargo miri test)`.

I am keeping `-Zmiri-env-forward` since it is still useful, e.g. to have RUST_BACKTRACE=full set in an otherwise deterministic execution.

`@rust-lang/miri` any objections to removing `-Zmiri-env-exclude`?

README.md

@@ -288,14 +288,9 @@ environment variable. We first document the most relevant and most commonly used
   execution with a "permission denied" error being returned to the program.
   `warn` prints a full backtrace when that happens; `warn-nobacktrace` is less
   verbose. `hide` hides the warning entirely.
-* `-Zmiri-env-exclude=<var>` keeps the `var` environment variable isolated from the host so that it
-  cannot be accessed by the program. Can be used multiple times to exclude several variables. The
-  `TERM` environment variable is excluded by default in Windows to prevent the libtest harness from
-  accessing the file system. This has no effect unless `-Zmiri-disable-isolation` is also set.
 * `-Zmiri-env-forward=<var>` forwards the `var` environment variable to the interpreted program. Can
-  be used multiple times to forward several variables. This takes precedence over
-  `-Zmiri-env-exclude`: if a variable is both forwarded and exluced, it *will* get forwarded. This
-  means in particular `-Zmiri-env-forward=TERM` overwrites the default exclusion of `TERM`.
+  be used multiple times to forward several variables. Execution will still be deterministic if the
+  value of forwarded variables stays the same. Has no effect if `-Zmiri-disable-isolation` is set.
 * `-Zmiri-ignore-leaks` disables the memory leak checker, and also allows some
   remaining threads to exist when the main thread exits.
 * `-Zmiri-permissive-provenance` disables the warning for integer-to-pointer casts and

src/bin/miri.rs

@@ -441,8 +441,10 @@ fn main() {
                             "-Zmiri-seed should only contain valid hex digits [0-9a-fA-F] and must fit into a u64 (max 16 characters)"
                         ));
             miri_config.seed = Some(seed);
-        } else if let Some(param) = arg.strip_prefix("-Zmiri-env-exclude=") {
-            miri_config.excluded_env_vars.push(param.to_owned());
+        } else if let Some(_param) = arg.strip_prefix("-Zmiri-env-exclude=") {
+            show_error!(
+                "`-Zmiri-env-exclude` has been removed; unset env vars before starting Miri instead"
+            );
         } else if let Some(param) = arg.strip_prefix("-Zmiri-env-forward=") {
             miri_config.forwarded_env_vars.push(param.to_owned());
         } else if let Some(param) = arg.strip_prefix("-Zmiri-track-pointer-tag=") {

src/eval.rs

@@ -74,7 +74,7 @@ pub enum BacktraceStyle {
 #[derive(Clone)]
 pub struct MiriConfig {
     /// The host environment snapshot to use as basis for what is provided to the interpreted program.
-    /// (This is still subject to isolation as well as `excluded_env_vars` and `forwarded_env_vars`.)
+    /// (This is still subject to isolation as well as `forwarded_env_vars`.)
     pub env: Vec<(OsString, OsString)>,
     /// Determine if validity checking is enabled.
     pub validate: bool,
@@ -88,8 +88,6 @@ pub struct MiriConfig {
     pub isolated_op: IsolatedOp,
     /// Determines if memory leaks should be ignored.
     pub ignore_leaks: bool,
-    /// Environment variables that should always be isolated from the host.
-    pub excluded_env_vars: Vec<String>,
     /// Environment variables that should always be forwarded from the host.
     pub forwarded_env_vars: Vec<String>,
     /// Command-line arguments passed to the interpreted program.
@@ -146,7 +144,6 @@ impl Default for MiriConfig {
             check_abi: true,
             isolated_op: IsolatedOp::Reject(RejectOpWith::Abort),
             ignore_leaks: false,
-            excluded_env_vars: vec![],
             forwarded_env_vars: vec![],
             args: vec![],
             seed: None,

src/shims/env.rs

@@ -42,19 +42,12 @@ impl<'tcx> EnvVars<'tcx> {
         config: &MiriConfig,
     ) -> InterpResult<'tcx> {
         let target_os = ecx.tcx.sess.target.os.as_ref();
-        let mut excluded_env_vars = config.excluded_env_vars.clone();
-        if target_os == "windows" {
-            // HACK: Exclude `TERM` var to avoid terminfo trying to open the termcap file.
-            excluded_env_vars.push("TERM".to_owned());
-        }
 
         // Skip the loop entirely if we don't want to forward anything.
         if ecx.machine.communicate() || !config.forwarded_env_vars.is_empty() {
             for (name, value) in &config.env {
-                // Always forward what is in `forwarded_env_vars`; that list can take precedence over excluded_env_vars.
-                let forward = config.forwarded_env_vars.iter().any(|v| **v == *name)
-                    || (ecx.machine.communicate()
-                        && !excluded_env_vars.iter().any(|v| **v == *name));
+                let forward = ecx.machine.communicate()
+                    || config.forwarded_env_vars.iter().any(|v| **v == *name);
                 if forward {
                     let var_ptr = match target_os {
                         target if target_os_is_unix(target) =>

tests/pass/concurrency/sync.rs

@@ -81,7 +81,9 @@ fn check_conditional_variables_timed_wait_notimeout() {
         cvar.notify_one();
     });
 
-    let (_guard, timeout) = cvar.wait_timeout(guard, Duration::from_millis(1000)).unwrap();
+    // macOS runners are very unreliable.
+    let timeout = if cfg!(target_os = "macos") { 2000 } else { 500 };
+    let (_guard, timeout) = cvar.wait_timeout(guard, Duration::from_millis(timeout)).unwrap();
     assert!(!timeout.timed_out());
     handle.join().unwrap();
 }