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

bors · bors · commit 11f3f41d9224 · 2024-01-25T00:31:49.000Z
Actually abort in panic-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.

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.
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 = 3;
 
 #[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:
