Fix handling of panics in ORPC servers

Cargo.toml

@@ -91,10 +91,13 @@ while_let_loop = "allow"
 snafu = { version = "0.8", default-features = false, features = ["alloc", "rust_1_81"] }
 
 [profile.dev]
+# WARNING: This setting does not apply to no_std code. That code is forced to use unwind by osdk
+# passing `-C panic=unwind` to ructc.
 panic = "unwind"
 
 [profile.release]
 lto = "thin"
+# WARNING: See above
 panic = "unwind"
 
 # Release profile configuration with Link Time Optimization (LTO) enabled.
@@ -107,6 +110,7 @@ panic = "unwind"
 inherits = "release"
 lto = true
 # lto can only be enabled when panic strategy is abort
+# TODO(arthurp): See `panic = "unwind"` notes above. This may break LTO.
 panic = "abort"
 # set codegen-units as the smallest number
 codegen-units = 1

kernel/src/thread/oops.rs

@@ -11,18 +11,17 @@
 //! to make Rust panics as a general exception handling mechanism. Handling
 //! exceptions with [`Result`] is more idiomatic.
 
-use alloc::{
-    boxed::Box,
-    format,
-    string::{String, ToString},
-    sync::Arc,
-};
+use alloc::{boxed::Box, format, string::String, sync::Arc};
 use core::{
     result::Result,
     sync::atomic::{AtomicBool, AtomicUsize, Ordering},
 };
 
-use ostd::{cpu::PinCurrentCpu, panic, task::disable_preempt};
+use ostd::{
+    panic::{self, CaughtPanic},
+    stack_info::{Location, StackInfo},
+    task::disable_preempt,
+};
 
 use super::Thread;
 
@@ -47,7 +46,7 @@ pub struct OopsInfo {
 ///
 /// If the kernel is configured to panic on oops, this function will not return
 /// when a oops happens.
-pub fn catch_panics_as_oops<F, R>(f: F) -> Result<R, OopsInfo>
+pub fn catch_panics_as_oops<F, R>(f: F) -> Result<R, CaughtPanic>
 where
     F: FnOnce() -> R,
 {
@@ -56,9 +55,9 @@ where
     match result {
         Ok(result) => Ok(result),
         Err(err) => {
-            let info = err.downcast::<OopsInfo>().unwrap();
+            let caught_panic = err.downcast::<CaughtPanic>().unwrap();
 
-            log::error!("Oops! {}", info.message);
+            log::error!("Oops! {}", caught_panic);
 
             let count = OOPS_COUNT.fetch_add(1, Ordering::Relaxed);
             if count >= MAX_OOPS_COUNT {
@@ -67,7 +66,7 @@ where
                 panic::abort();
             }
 
-            Err(*info)
+            Err(*caught_panic)
         }
     }
 }
@@ -86,39 +85,29 @@ fn panic_handler(info: &core::panic::PanicInfo) -> ! {
     if let Some(thread) = Thread::current() {
         let panic_on_oops = PANIC_ON_OOPS.load(Ordering::Relaxed);
         if !panic_on_oops && info.can_unwind() {
-            // TODO: eliminate the need for heap allocation.
-            let message = if let Some(location) = info.location() {
-                format!("{} at {}:{}", message, location.file(), location.line())
-            } else {
-                message.to_string()
-            };
-            // Raise the panic and expect it to be caught.
-            panic::begin_panic(Box::new(OopsInfo { message, thread }));
+            // skip 3 frames: this function, __ostd_panic_handler, rust_begin_unwind
+            let mut stack_info = StackInfo::new(3);
+            stack_info.location = info.location().map(|l| Location::from_location(l));
+
+            let r = panic::begin_panic(Box::new(ostd::panic::CaughtPanic {
+                message: format!("{}", message),
+                context: Some(stack_info),
+            }));
         }
     }
 
     let preempt_guard = disable_preempt();
     let thread = Thread::current();
-    let cpu = preempt_guard.current_cpu();
+    let context = StackInfo::new(1);
 
     // Halt the system if the panic is not caught.
-    if let Some(location) = info.location() {
-        log::error!(
-            "Uncaught panic:\n\t{}\n\tat {}:{}\n\ton CPU {} by thread {:?}",
-            message,
-            location.file(),
-            location.line(),
-            cpu.as_usize(),
-            thread,
-        );
-    } else {
-        log::error!(
-            "Uncaught panic:\n\t{}\n\ton CPU {} by thread {:?}",
-            message,
-            cpu.as_usize(),
-            thread,
-        );
-    }
+
+    log::error!(
+        "Uncaught panic:\n\t{}\n\t{}\n\tby thread {:?}",
+        message,
+        context,
+        thread,
+    );
 
     if info.can_unwind() {
         panic::print_stack_trace();

osdk/src/commands/build/bin.rs

@@ -217,7 +217,7 @@ fn install_setup_with_arch(
         SetupInstallArch::Other(path) => path.to_str().unwrap(),
     });
     cmd.arg("-Zbuild-std=core,alloc,compiler_builtins");
-    cmd.arg("-Zbuild-std-features=compiler-builtins-mem");
+    cmd.arg("-Zbuild-std-features=compiler-builtins-mem,panic-unwind");
     // Specify the build target directory to avoid cargo running
     // into a deadlock reading the workspace files.
     cmd.arg("--target-dir").arg(target_dir.as_os_str());

osdk/src/commands/build/mod.rs

@@ -214,8 +214,14 @@ fn build_kernel_elf(
         &rustc_linker_script_arg,
         "-C relocation-model=static",
         "-C relro-level=off",
-        // Even if we disabled unwinding on panic, we need to specify this to show backtraces.
-        "-C force-unwind-tables=yes",
+        // OSDK kernels always support unwinding on panic. Ideally, it would be optional, however
+        // this has to be forced because the underlying no_std target will force the panic mode to
+        // abort.
+        //
+        // NOTE: This is a difference from upstream Asterinas because ORPC uses catch_unwind to
+        // enable failure isolation between servers. Without this, that isolation will work
+        // *sometimes*, but not always.
+        "-C panic=unwind",
         // This is to let rustc know that "cfg(ktest)" is our well-known configuration.
         // See the [Rust Blog](https://blog.rust-lang.org/2024/05/06/check-cfg.html) for details.
         "--check-cfg cfg(ktest)",

osdk/src/commands/util.rs

@@ -6,7 +6,7 @@ use crate::util::{get_kernel_crate, new_command_checked_exists};
 
 pub const COMMON_CARGO_ARGS: &[&str] = &[
     "-Zbuild-std=core,alloc,compiler_builtins",
-    "-Zbuild-std-features=compiler-builtins-mem",
+    "-Zbuild-std-features=compiler-builtins-mem,panic-unwind",
 ];
 
 pub const DEFAULT_TARGET_RELPATH: &str = "osdk";

ostd/src/orpc/framework/mod.rs

@@ -36,6 +36,7 @@ use core::{
     sync::atomic::{AtomicBool, AtomicUsize, Ordering},
 };
 
+use log::error;
 pub use threads::spawn_thread;
 
 use crate::{
@@ -153,9 +154,10 @@ impl ServerBase {
         }) {
             Ok(ret) => ret,
             Err(payload) => {
-                let e = RPCError::from_panic(payload);
-                self.abort(&e);
-                Err(e.into())
+                let err = RPCError::from_panic(payload);
+                error!("ORPC method call panicked: {}", err);
+                self.abort(&err);
+                Err(err.into())
             }
         }
     }

ostd/src/orpc_common/errors.rs

@@ -1,13 +1,16 @@
 // SPDX-License-Identifier: MPL-2.0
 //! Error module for ORPC
 
-use alloc::string::{String, ToString};
+use alloc::{
+    format,
+    string::{String, ToString},
+};
 use core::any::Any;
 
 use ostd_macros::ostd_error;
 use snafu::Snafu;
 
-use crate::prelude::Box;
+use crate::{panic::CaughtPanic, prelude::Box, stack_info::StackInfo};
 
 /// An error during an RPC call: failure via panic and the server not running.
 ///
@@ -23,7 +26,7 @@ pub enum RPCError {
     /// it cannot be, then the string will be a generic error message.
     #[snafu(display("{message} ({context})"))]
     Panic {
-        /// message associated with this panic
+        /// The formatted panic message, including source file and line.
         message: String,
     },
     /// The server does not exist or is not running. This can happen when a server already crashed or has been shutdown.
@@ -32,29 +35,50 @@ pub enum RPCError {
 }
 
 /// Convert a payload to a string if possible. This simply performs downcasts.
-fn payload_as_str(payload: &dyn Any) -> Option<&str> {
-    if let Some(s) = payload.downcast_ref::<&str>() {
-        Some(s)
-    } else if let Some(s) = payload.downcast_ref::<String>() {
-        Some(s)
-    } else {
-        None
-    }
+fn payload_as_caught_panic(payload: Box<dyn Any + Send + 'static>) -> Option<CaughtPanic> {
+    // Attempt downcasts to 4 different types: &str, String, CaughtPanic, ostd_test::PanicInfo
+    let payload = payload
+        .downcast::<&str>()
+        .map(|s| CaughtPanic {
+            message: s.to_string(),
+            context: None,
+        })
+        .or_else(|payload| {
+            payload.downcast::<String>().map(|s| CaughtPanic {
+                message: *s,
+                context: None,
+            })
+        })
+        .or_else(|payload| payload.downcast::<CaughtPanic>().map(|c| *c))
+        .or_else(|payload| {
+            payload
+                .downcast::<ostd_test::PanicInfo>()
+                .map(|c| (*c).into())
+        });
+    payload.ok()
 }
 
 impl RPCError {
     /// Convert a panic payload into an RPCError.
     ///
     /// This take ownership of the payload to allow it to be implemented allocation free in as many cases as possible.
     /// This is important since allocating on an error path can cause issues.
-    pub fn from_panic(payload: Box<dyn Any>) -> Self {
-        // TODO(#72): The `to_string` call could potentially allocate which could be an issue on the panic path. A better way
-        // to do this may be needed.
-        PanicSnafu {
-            message: payload_as_str(payload.as_ref())
-                .unwrap_or("Non-string panic payload")
-                .to_string(),
+    pub fn from_panic(payload: Box<dyn Any + Send + 'static>) -> Self {
+        let panic = payload_as_caught_panic(payload).unwrap_or_else(|| CaughtPanic {
+            message: "[unknown panic payload]".to_string(),
+            context: None,
+        });
+        let (message, context) = if let Some(context) = panic.context {
+            (panic.message, context)
+        } else {
+            (
+                format!("{} (with error conversion context)", panic.message),
+                StackInfo::new(0),
+            )
+        };
+        RPCError::Panic {
+            message,
+            context: Box::new(context),
         }
-        .build()
     }
 }

ostd/src/panic.rs

@@ -2,13 +2,17 @@
 
 //! Panic support.
 
-use core::ffi::c_void;
+use alloc::string::String;
+use core::{ffi::c_void, fmt::Display};
 
 pub use unwinding::panic::{begin_panic, catch_unwind};
 
 use crate::{
     arch::qemu::{QemuExitCode, exit_qemu},
+    cpu::CpuId,
     early_print, early_println,
+    stack_info::StackInfo,
+    stacktrace::CapturedStackTrace,
     sync::SpinLock,
 };
 
@@ -21,6 +25,45 @@ use unwinding::abi::{
     UnwindReasonCode,
 };
 
+/// A panic payload that carries a human-readable message and context information at the panic site.
+#[derive(Debug, Clone)]
+pub struct CaughtPanic {
+    /// The formatted panic message. This does not include the file and line information as that is
+    /// in the context below.
+    pub message: String,
+    /// Stack trace captured at the panic site.
+    pub context: Option<StackInfo>,
+}
+
+impl From<ostd_test::PanicInfo> for CaughtPanic {
+    fn from(value: ostd_test::PanicInfo) -> Self {
+        Self {
+            message: value.message,
+            context: Some(StackInfo {
+                stack_trace: CapturedStackTrace::empty(),
+                task_id: None,
+                cpu_id: CpuId::current_racy(),
+                server_id: None,
+                location: Some(crate::stack_info::Location {
+                    file: value.file.into(),
+                    line: value.line as u32,
+                    column: value.col as u32,
+                }),
+            }),
+        }
+    }
+}
+
+impl Display for CaughtPanic {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        if let Some(context) = &self.context {
+            f.write_fmt(format_args!("Panic: {} ({})", self.message, context))
+        } else {
+            f.write_fmt(format_args!("Panic: {} (no context)", self.message))
+        }
+    }
+}
+
 /// The default panic handler for OSTD based kernels.
 ///
 /// The user can override it by defining their own panic handler with the macro