diff --git a/Cargo.toml b/Cargo.toml index 4750a0f90..ef0ec84d5 100644 --- a/Cargo.toml +++ b/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 diff --git a/kernel/src/thread/oops.rs b/kernel/src/thread/oops.rs index 3f62d24aa..186712e5b 100644 --- a/kernel/src/thread/oops.rs +++ b/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: F) -> Result +pub fn catch_panics_as_oops(f: F) -> Result where F: FnOnce() -> R, { @@ -56,9 +55,9 @@ where match result { Ok(result) => Ok(result), Err(err) => { - let info = err.downcast::().unwrap(); + let caught_panic = err.downcast::().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(); diff --git a/osdk/src/commands/build/bin.rs b/osdk/src/commands/build/bin.rs index c0c796d96..4c5923deb 100644 --- a/osdk/src/commands/build/bin.rs +++ b/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()); diff --git a/osdk/src/commands/build/mod.rs b/osdk/src/commands/build/mod.rs index d38b51eb3..99ff2d95c 100644 --- a/osdk/src/commands/build/mod.rs +++ b/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)", diff --git a/osdk/src/commands/util.rs b/osdk/src/commands/util.rs index 1b521791c..253729ca2 100644 --- a/osdk/src/commands/util.rs +++ b/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"; diff --git a/ostd/src/orpc/framework/mod.rs b/ostd/src/orpc/framework/mod.rs index 0d031a9c2..d5821744f 100644 --- a/ostd/src/orpc/framework/mod.rs +++ b/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()) } } } diff --git a/ostd/src/orpc_common/errors.rs b/ostd/src/orpc_common/errors.rs index a3d70f6d3..377718aa7 100644 --- a/ostd/src/orpc_common/errors.rs +++ b/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,14 +35,27 @@ 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::() { - Some(s) - } else { - None - } +fn payload_as_caught_panic(payload: Box) -> Option { + // 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::().map(|s| CaughtPanic { + message: *s, + context: None, + }) + }) + .or_else(|payload| payload.downcast::().map(|c| *c)) + .or_else(|payload| { + payload + .downcast::() + .map(|c| (*c).into()) + }); + payload.ok() } impl RPCError { @@ -47,14 +63,22 @@ impl 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) -> 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) -> 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() } } diff --git a/ostd/src/panic.rs b/ostd/src/panic.rs index bb4d8cad5..c4583e0d5 100644 --- a/ostd/src/panic.rs +++ b/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, +} + +impl From 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 diff --git a/ostd/src/stack_info.rs b/ostd/src/stack_info.rs index 5b5c72fd3..5351e19f8 100644 --- a/ostd/src/stack_info.rs +++ b/ostd/src/stack_info.rs @@ -2,12 +2,75 @@ //! Tools for capturing information about the stack execution context. -use core::{fmt::Display, num::NonZeroUsize, panic::Location}; +use alloc::{ + borrow::{Cow, ToOwned}, + fmt, +}; +use core::{fmt::Display, num::NonZeroUsize}; use snafu::GenerateImplicitData; use crate::{cpu::CpuId, stacktrace::CapturedStackTrace, task::Task}; +/// A location in the code. Unlike, [`snafu::Location`] and [`core::panic::Location`], this can hold +/// a dynamic string. However, if given a `&'static str`, it will not allocate memory (internally it +/// uses [`Cow`]). +#[derive(Clone, Debug)] +pub struct Location { + /// The file where the error was reported. + /// + /// This can be either a `&'static str` or a `String`. + pub file: Cow<'static, str>, + /// The line where the error was reported + pub line: u32, + /// The column where the error was reported + pub column: u32, +} + +impl fmt::Display for Location { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{file}:{line}:{column}", + file = self.file, + line = self.line, + column = self.column, + ) + } +} + +impl From<&snafu::Location> for Location { + fn from(value: &snafu::Location) -> Self { + Self { + file: value.file.into(), + line: value.line, + column: value.column, + } + } +} + +impl From<&core::panic::Location<'static>> for Location { + fn from(value: &core::panic::Location<'static>) -> Self { + Self { + file: value.file().into(), + line: value.line(), + column: value.column(), + } + } +} + +impl Location { + /// Create a [`Location`] from a [`core::panic::Location`] with a limited lifetime. This + /// allocates memory in which to store the filename. + pub fn from_location(value: &core::panic::Location<'_>) -> Self { + Self { + file: value.file().to_owned().into(), + line: value.line(), + column: value.column(), + } + } +} + /// The stack context in which an error may have occurred. This should be used in most error types. /// /// This type may also be useful for capturing the context of some other event for use in later @@ -18,7 +81,7 @@ use crate::{cpu::CpuId, stacktrace::CapturedStackTrace, task::Task}; /// NOTE: This module has some degree of "premature" optimization. This is because we want to /// encourage [`StackInfo`] to be used liberally so keeping the cost as low as possible has a /// very significant value. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Debug)] #[non_exhaustive] pub struct StackInfo { /// The stacktrace. @@ -34,10 +97,7 @@ pub struct StackInfo { /// occurred. pub server_id: Option, /// The source location. - /// - /// NOTE: We use `core::panic::Location` instead of `snafu::Location`. `core::panic::Location` - /// can be stored as a single pointer making this field 8-bytes instead of 24-bytes. - pub location: Option<&'static Location<'static>>, + pub location: Option, } impl StackInfo { @@ -62,7 +122,7 @@ impl StackInfo { task_id, cpu_id: CpuId::current_racy(), server_id, - location: Some(Location::caller()), + location: Some(core::panic::Location::caller().into()), } } @@ -80,7 +140,7 @@ impl StackInfo { impl Display for StackInfo { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - if let Some(location) = self.location { + if let Some(location) = &self.location { write!(f, "at {location} ")?; } if let Some(tid) = self.task_id { @@ -106,8 +166,6 @@ impl GenerateImplicitData for StackInfo { #[cfg(ktest)] mod test { - use alloc::borrow::ToOwned; - use super::*; use crate::prelude::*; @@ -138,8 +196,8 @@ and generate a stack trace with the top frame assert_eq!(context.task_id.unwrap(), task.id()); assert!(context.server_id.is_none()); assert_eq!( - context.location.unwrap().file().to_owned(), - Location::caller().file().to_owned() + context.location.unwrap().file.as_ref(), + core::panic::Location::caller().file() ); } diff --git a/ostd/tests/early-boot-test-kernel/Cargo.toml b/ostd/tests/early-boot-test-kernel/Cargo.toml index 7c5818367..0508b0111 100644 --- a/ostd/tests/early-boot-test-kernel/Cargo.toml +++ b/ostd/tests/early-boot-test-kernel/Cargo.toml @@ -8,9 +8,5 @@ license = "MPL-2.0" [dependencies] ostd = { path = "../.." } - -[profile.dev] -panic = "unwind" - [lints] workspace = true