Auto merge of #120326 - tmandry:abort-in-tests, r=<try>

bors · bors · commit 765d6e6ebd88 · 2024-01-26T01:09:20.000Z
Actually abort in -Zpanic-abort-tests

When a test fails in panic=abort, it can be useful to have a debugger or other tooling hook into the `abort()` call for debugging. Doing this some other way would require it to hard code details of Rust's panic machinery.

There's no reason we couldn't have done this in the first place; using a single exit code for "success" or "failed" was just simpler. Now we are aware of the special exit codes for posix and windows platforms, logging a special error if an unrecognized code is used on those platforms, and falling back to just "failure" on other platforms.

This continues to account for `#[should_panic]` inside the test process itself, so there's no risk of misinterpreting a random call to `abort()` as an expected panic. Any exit code besides `TR_OK` is logged as a test failure.

As an added benefit, this would allow us to support panic=immediate_abort (but not `#[should_panic]`), without noise about unexpected exit codes when a test fails.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -571,6 +571,11 @@ jobs:
             env:
               CODEGEN_BACKENDS: "llvm,cranelift"
             os: ubuntu-20.04-16core-64gb
+          - name: x86_64-msvc
+            env:
+              RUST_CONFIGURE_ARGS: "--build=x86_64-pc-windows-msvc --enable-profiler"
+              SCRIPT: make ci-msvc
+            os: windows-2019-8core-32gb
     timeout-minutes: 600
     runs-on: "${{ matrix.os }}"
     steps:
diff --git a/library/test/src/helpers/exit_code.rs b/library/test/src/helpers/exit_code.rs
diff --git a/library/test/src/helpers/mod.rs b/library/test/src/helpers/mod.rs
@@ -2,6 +2,5 @@
 //! but used in `libtest`.
 
 pub mod concurrency;
-pub mod exit_code;
 pub mod metrics;
 pub mod shuffle;
diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs
@@ -85,7 +85,6 @@ mod tests;
 use core::any::Any;
 use event::{CompletedTest, TestEvent};
 use helpers::concurrency::get_concurrency;
-use helpers::exit_code::get_exit_code;
 use helpers::shuffle::{get_shuffle_seed, shuffle_tests};
 use options::RunStrategy;
 use test_result::*;
@@ -712,17 +711,7 @@ fn spawn_test_subprocess(
         formatters::write_stderr_delimiter(&mut test_output, &desc.name);
         test_output.extend_from_slice(&stderr);
 
-        let result = match (|| -> Result<TestResult, String> {
-            let exit_code = get_exit_code(status)?;
-            Ok(get_result_from_exit_code(&desc, exit_code, &time_opts, &exec_time))
-        })() {
-            Ok(r) => r,
-            Err(e) => {
-                write!(&mut test_output, "Unexpected error: {e}").unwrap();
-                TrFailed
-            }
-        };
-
+        let result = get_result_from_exit_code(&desc, status, &time_opts, &exec_time);
         (result, test_output, exec_time)
     })();
 
@@ -751,7 +740,7 @@ fn run_test_in_spawned_subprocess(desc: TestDesc, runnable_test: RunnableTest) -
         if let TrOk = test_result {
             process::exit(test_result::TR_OK);
         } else {
-            process::exit(test_result::TR_FAILED);
+            process::abort();
         }
     });
     let record_result2 = record_result.clone();
diff --git a/library/test/src/test_result.rs b/library/test/src/test_result.rs
@@ -1,4 +1,8 @@
 use std::any::Any;
+use std::process::ExitStatus;
+
+#[cfg(unix)]
+use std::os::unix::process::ExitStatusExt;
 
 use super::bench::BenchSamples;
 use super::options::ShouldPanic;
@@ -7,11 +11,16 @@ use super::types::TestDesc;
 
 pub use self::TestResult::*;
 
-// Return codes for secondary process.
+// Return code for secondary process.
 // Start somewhere other than 0 so we know the return code means what we think
 // it means.
 pub const TR_OK: i32 = 50;
-pub const TR_FAILED: i32 = 51;
+
+#[cfg(unix)]
+const SIGABRT: i32 = 6;
+
+#[cfg(windows)]
+const EXIT_ABORTED: i32 = -1073740791;
 
 #[derive(Debug, Clone, PartialEq)]
 pub enum TestResult {
@@ -81,14 +90,28 @@ pub fn calc_result<'a>(
 /// Creates a `TestResult` depending on the exit code of test subprocess.
 pub fn get_result_from_exit_code(
     desc: &TestDesc,
-    code: i32,
+    status: ExitStatus,
     time_opts: &Option<time::TestTimeOptions>,
     exec_time: &Option<time::TestExecTime>,
 ) -> TestResult {
-    let result = match code {
-        TR_OK => TestResult::TrOk,
-        TR_FAILED => TestResult::TrFailed,
-        _ => TestResult::TrFailedMsg(format!("got unexpected return code {code}")),
+    let result = match status.code() {
+        Some(TR_OK) => TestResult::TrOk,
+        #[cfg(windows)]
+        Some(EXIT_ABORTED) => TestResult::TrFailed,
+        #[cfg(unix)]
+        None => match status.signal() {
+            Some(SIGABRT) => TestResult::TrFailed,
+            Some(signal) => {
+                TestResult::TrFailedMsg(format!("child process exited with signal {signal}"))
+            }
+            None => unreachable!("status.code() returned None but status.signal() was None"),
+        },
+        #[cfg(not(unix))]
+        None => TestResult::TrFailedMsg(format!("unknown return code")),
+        #[cfg(any(windows, unix))]
+        Some(code) => TestResult::TrFailedMsg(format!("got unexpected return code {code}")),
+        #[cfg(not(any(windows, unix)))]
+        Some(_) => TestResult::TrFailed,
     };
 
     // If test is already failed (or allowed to fail), do not change the result.
diff --git a/src/ci/github-actions/ci.yml b/src/ci/github-actions/ci.yml
@@ -729,6 +729,11 @@ jobs:
             env:
               CODEGEN_BACKENDS: llvm,cranelift
             <<: *job-linux-16c
+          - name: x86_64-msvc
+            env:
+              RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-msvc --enable-profiler
+              SCRIPT: make ci-msvc
+            <<: *job-windows-8c
 
 
   master:
