From db2fd9db913ca660b42f930c7190ba7cee14167f Mon Sep 17 00:00:00 2001
From: Patrick Mooney <pmooney@pfmooney.com>
Date: Thu, 21 Sep 2023 17:36:54 -0500
Subject: [PATCH] Revert abort-on-panic in 'dev' cargo profile

Since proper unwinding is required for handling `should_panic` tests
cases, the existing default of abort-on-panic was rather inconvenient.
This returns the 'dev' profile to using unwind-on-panic, and cleans up
the 'phd' profile, which can use 'dev' as well for its unwinding.
---
 .github/buildomat/jobs/image.sh     |  3 +++
 .github/buildomat/jobs/phd-build.sh | 10 +++++-----
 Cargo.toml                          | 15 ++++++++-------
 phd-tests/README.md                 | 10 +++++-----
 phd-tests/quickstart.sh             |  2 +-
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/.github/buildomat/jobs/image.sh b/.github/buildomat/jobs/image.sh
index bbbe80607..d66ee29f0 100755
--- a/.github/buildomat/jobs/image.sh
+++ b/.github/buildomat/jobs/image.sh
@@ -30,6 +30,9 @@ banner build
 
 # Enable the "omicron-build" feature to indicate this is an artifact destined
 # for production use on an appropriately configured Oxide machine
+#
+# The 'release' profile is configured for abort-on-panic, so we get an
+# immediate coredump rather than unwinding in the case of an error.
 ptime -m cargo build --release --verbose -p propolis-server --features omicron-build
 
 banner image
diff --git a/.github/buildomat/jobs/phd-build.sh b/.github/buildomat/jobs/phd-build.sh
index e27cc0fd4..f591748dd 100644
--- a/.github/buildomat/jobs/phd-build.sh
+++ b/.github/buildomat/jobs/phd-build.sh
@@ -17,15 +17,15 @@ outdir="/out"
 cargo --version
 rustc --version
 
-# Build the Propolis server binary in debug mode to enable assertions
-# that should fire during tests.
+# Build the Propolis server binary with 'dev' profile to enable assertions that
+# should fire during tests.
 banner build-propolis
 ptime -m cargo build --verbose -p propolis-server
 
-# Build the PHD runner with the phd profile to enable unwind on panic,
-# which the framework requires to catch certain test failures.
+# The PHD runner requires unwind-on-panic to catch certain test failures, so
+# build it with the 'dev' profile which is so configured.
 banner build-phd
-ptime -m cargo build --verbose -p phd-runner --profile=phd
+ptime -m cargo build --verbose -p phd-runner
 
 banner contents
 tar -czvf target/debug/propolis-server-debug.tar.gz \
diff --git a/Cargo.toml b/Cargo.toml
index 131313c99..c01029f95 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,17 +27,18 @@ exclude = [
   "phd-tests/buildomat",
 ]
 
-[profile.dev]
+# If one wants the 'dev' profile, but with "panic = abort" semantics, they
+# should opt in with this profile.  Unwinding is required by PHD and
+# should_abort cargo tests, and so remains the default for the 'dev' profile.
+[profile.dev-abort]
+inherits = "dev"
 panic = "abort"
+
+# Building for 'release' implies running on a real illumos system, where we
+# certainly want (rust) panics to cause an immediate abort and coredump.
 [profile.release]
 panic = "abort"
 
-# The PHD test runner needs to use unwinding to catch panics that occur during
-# tests (e.g. due to a failed `assert!` in a test case).
-[profile.phd]
-inherits = "dev"
-panic = "unwind"
-
 [workspace.dependencies]
 # Internal crates
 bhyve_api = { path = "crates/bhyve-api" }
diff --git a/phd-tests/README.md b/phd-tests/README.md
index 9013654f9..38bdd5b92 100644
--- a/phd-tests/README.md
+++ b/phd-tests/README.md
@@ -34,15 +34,15 @@ against them.
 
 To build:
 
-`cargo build -p phd-runner --profile=phd`
+`cargo build -p phd-runner
 
-Note that `--profile=phd` is required to allow the runner to catch assertions
-from test cases (the default Propolis profile aborts on panic instead of
-unwinding).
+PHD requires the unwinding of stacks in order to properly catch assertions in
+test cases, so building with a profile which sets `panic = "abort"` is not
+supported.  This precludes the use of the `release` or `dev-abort` profiles.
 
 To run:
 
-`pfexec cargo run -p phd-runner --profile=phd -- [OPTIONS]`
+`pfexec cargo run -p phd-runner -- [OPTIONS]`
 
 Running under pfexec is required to allow PHD to ensure the host system is
 correctly configured to run live migration tests.
diff --git a/phd-tests/quickstart.sh b/phd-tests/quickstart.sh
index 9f19814b9..75f00fe17 100755
--- a/phd-tests/quickstart.sh
+++ b/phd-tests/quickstart.sh
@@ -10,7 +10,7 @@ if [ ! -d "$PHD_QUICKSTART_DIR" ]; then
 	mkdir $PHD_QUICKSTART_DIR
 fi
 
-pfexec cargo run --profile=phd -p phd-runner -- \
+pfexec cargo run -p phd-runner -- \
 	run \
 	--artifact-toml-path ./artifacts.toml \
 	--tmp-directory $PHD_QUICKSTART_DIR \