From 5efec7922a40a2cf7a31f069fda4d06adffbcc3f Mon Sep 17 00:00:00 2001 From: James Carl Date: Tue, 21 Mar 2023 21:28:40 -0400 Subject: [PATCH 1/2] An attempt at a more advanced panic logger. --- crates/fj-app/src/main.rs | 4 ++ crates/fj/src/abi/mod.rs | 91 ++++++++++++++++++++++++++++++++++----- 2 files changed, 85 insertions(+), 10 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 1b4c51128..ffa435faa 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -19,6 +19,7 @@ mod path; use std::{env, error::Error}; use anyhow::{anyhow, Context}; +use fj::abi; use fj_export::export; use fj_host::Parameters; use fj_operations::shape_processor::ShapeProcessor; @@ -36,6 +37,9 @@ fn main() -> anyhow::Result<()> { .event_format(format().pretty()) .init(); + // We need to initialize panic handling before attempting to load a model. + abi::initialize_panic_handling(); + let args = Args::parse(); let config = Config::load()?; let model_path = ModelPath::from_args_and_config(&args, &config); diff --git a/crates/fj/src/abi/mod.rs b/crates/fj/src/abi/mod.rs index 5a237bc94..f31847a07 100644 --- a/crates/fj/src/abi/mod.rs +++ b/crates/fj/src/abi/mod.rs @@ -43,7 +43,7 @@ mod host; mod metadata; mod model; -use std::any::Any; +use std::{any::Any, fmt::Display, panic, sync::Mutex}; pub use self::{ context::Context, @@ -118,15 +118,86 @@ pub type ModelMetadataResult = /// pub const INIT_FUNCTION_NAME: &str = "fj_model_init"; -fn on_panic(payload: Box) -> crate::abi::ffi_safe::String { - let msg: &str = - if let Some(s) = payload.downcast_ref::() { - s.as_str() - } else if let Some(s) = payload.downcast_ref::<&str>() { - s +// Contains details about a panic that we need to pass back to the application from the panic hook. +struct PanicInfo { + message: Option, + location: Option, +} + +impl Display for PanicInfo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let message = self + .message + .as_ref() + .map_or("No error given", |message| message.as_str()); + + write!(f, "{}, ", message)?; + + if let Some(location) = self.location.as_ref() { + write!(f, "{}", location)?; + } else { + write!(f, "No location given")?; + } + + Ok(()) + } +} + +struct Location { + file: String, + line: u32, + column: u32, +} + +impl Display for Location { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}:{}:{}", self.file, self.line, self.column) + } +} + +static LAST_PANIC: Mutex> = Mutex::new(None); + +/// Capturing panics is something Rust really doesn't want you to do, and as such, they make it convoluted. +/// This sets up all the machinery in the background to pull it off. +/// +/// It's okay to call this multiple times. +pub fn initialize_panic_handling() { + panic::set_hook(Box::new(|panic_info| { + let mut last_panic = + LAST_PANIC.lock().expect("Panic queue was poisoned."); // FIXME that can probably overflow the stack. + let message = if let Some(s) = + panic_info.payload().downcast_ref::() + { + Some(s.as_str()) } else { - "A panic occurred" - }; + panic_info.payload().downcast_ref::<&str>().copied() + } + .map(|s| s.to_string()); - crate::abi::ffi_safe::String::from(msg.to_string()) + let location = panic_info.location().map(|location| Location { + file: location.file().to_string(), + line: location.line(), + column: location.column(), + }); + + *last_panic = Some(PanicInfo { message, location }); + })); +} + +fn on_panic(_payload: Box) -> crate::abi::ffi_safe::String { + // The payload is technically no longer needed, but I left it there just in case a change to `catch_unwind` made + // it useful again. + if let Ok(mut panic_info) = LAST_PANIC.lock() { + if let Some(panic_info) = panic_info.take() { + crate::abi::ffi_safe::String::from(format!("{}", panic_info)) + } else { + crate::abi::ffi_safe::String::from( + "Panic in model: No details were given.".to_string(), + ) + } + } else { + crate::abi::ffi_safe::String::from( + "Panic in model, but due to a poisoned panic queue the information could not be collected." + .to_string()) + } } From 4c5bdb9e73fdc3cf6886b1ac2d4e2a67b982bfdf Mon Sep 17 00:00:00 2001 From: James Carl Date: Wed, 22 Mar 2023 22:06:40 -0400 Subject: [PATCH 2/2] Moved panic handler location to correct location. --- crates/fj-app/src/main.rs | 4 --- crates/fj-proc/src/lib.rs | 62 +++++++++++++++++++++++++++++++++++++-- crates/fj/src/abi/mod.rs | 9 ++++-- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index ffa435faa..1b4c51128 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -19,7 +19,6 @@ mod path; use std::{env, error::Error}; use anyhow::{anyhow, Context}; -use fj::abi; use fj_export::export; use fj_host::Parameters; use fj_operations::shape_processor::ShapeProcessor; @@ -37,9 +36,6 @@ fn main() -> anyhow::Result<()> { .event_format(format().pretty()) .init(); - // We need to initialize panic handling before attempting to load a model. - abi::initialize_panic_handling(); - let args = Args::parse(); let config = Config::load()?; let model_path = ModelPath::from_args_and_config(&args, &config); diff --git a/crates/fj-proc/src/lib.rs b/crates/fj-proc/src/lib.rs index a97357488..6566e247e 100644 --- a/crates/fj-proc/src/lib.rs +++ b/crates/fj-proc/src/lib.rs @@ -2,7 +2,11 @@ mod expand; mod parse; use proc_macro::TokenStream; -use syn::{parse_macro_input, FnArg, ItemFn}; +use proc_macro2::{Ident, Span}; +use syn::{ + parse_macro_input, punctuated::Punctuated, token::Paren, Expr, ExprCall, + ExprPath, FnArg, ItemFn, PathArguments, PathSegment, Stmt, +}; /// Define a function-based model. /// @@ -91,13 +95,67 @@ pub fn model(_: TokenStream, input: TokenStream) -> TokenStream { match parse::parse(&item) { Ok(init) => { - let item = without_param_attrs(item); + let mut item = without_param_attrs(item); + + // Yes, all of this is to add `fj::abi::initialize_panic_handling();` to the top of the function. + item.block.stmts.insert( + 0, + Stmt::Semi( + Expr::Call(ExprCall { + attrs: vec![], + func: Box::new(Expr::Path(ExprPath { + attrs: vec![], + qself: None, + path: syn::Path { + leading_colon: None, + segments: { + let mut segments = Punctuated::new(); + + segments.push(PathSegment { + ident: Ident::new( + "fj", + Span::call_site(), + ), + arguments: PathArguments::None, + }); + + segments.push(PathSegment { + ident: Ident::new( + "abi", + Span::call_site(), + ), + arguments: PathArguments::None, + }); + + segments.push(PathSegment { + ident: Ident::new( + "initialize_panic_handling", + Span::call_site(), + ), + arguments: PathArguments::None, + }); + + segments + }, + }, + })), + paren_token: Paren { + span: Span::call_site(), + }, + args: Punctuated::new(), + }), + syn::token::Semi::default(), + ), + ); let tokens = quote::quote! { #item #init + }; + eprintln!("TOKENS: {}", tokens); + tokens.into() } Err(e) => e.into_compile_error().into(), diff --git a/crates/fj/src/abi/mod.rs b/crates/fj/src/abi/mod.rs index f31847a07..f142539d4 100644 --- a/crates/fj/src/abi/mod.rs +++ b/crates/fj/src/abi/mod.rs @@ -131,12 +131,12 @@ impl Display for PanicInfo { .as_ref() .map_or("No error given", |message| message.as_str()); - write!(f, "{}, ", message)?; + write!(f, "\"{}\", ", message)?; if let Some(location) = self.location.as_ref() { write!(f, "{}", location)?; } else { - write!(f, "No location given")?; + write!(f, "no location given")?; } Ok(()) @@ -189,7 +189,10 @@ fn on_panic(_payload: Box) -> crate::abi::ffi_safe::String { // it useful again. if let Ok(mut panic_info) = LAST_PANIC.lock() { if let Some(panic_info) = panic_info.take() { - crate::abi::ffi_safe::String::from(format!("{}", panic_info)) + crate::abi::ffi_safe::String::from(format!( + "Panic in model: {}", + panic_info + )) } else { crate::abi::ffi_safe::String::from( "Panic in model: No details were given.".to_string(),