Merge pull request #1094 from godot-rust/qol/refactor-storage

`bind/bind_mut` borrow errors now print previous stacktrace

Author: Bromeon

godot-core/src/meta/error/call_error.rs

@@ -301,7 +301,11 @@ impl CallError {
 
     #[doc(hidden)]
     pub fn failed_by_user_panic(call_ctx: &CallContext, reason: String) -> Self {
-        Self::new(call_ctx, reason, None)
+        // This can cause the panic message to be printed twice in some scenarios (e.g. bind_mut() borrow failure).
+        // But in other cases (e.g. itest `dynamic_call_with_panic`), it is only printed once.
+        // Would need some work to have a consistent experience.
+
+        Self::new(call_ctx, format!("function panicked: {reason}"), None)
     }
 
     fn new(

godot-core/src/private.rs

@@ -17,15 +17,17 @@ pub use sys::out;
 
 #[cfg(feature = "trace")]
 pub use crate::meta::trace;
+#[cfg(debug_assertions)]
+use std::cell::RefCell;
 
 use crate::global::godot_error;
 use crate::meta::error::CallError;
 use crate::meta::CallContext;
 use crate::sys;
-use std::cell::RefCell;
 use std::io::Write;
 use std::sync::atomic;
 use sys::Global;
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Global variables
 
@@ -249,6 +251,41 @@ pub fn format_panic_message(panic_info: &std::panic::PanicHookInfo) -> String {
     }
 }
 
+// Macro instead of function, to avoid 1 extra frame in backtrace.
+#[cfg(debug_assertions)]
+#[macro_export]
+macro_rules! format_backtrace {
+    ($prefix:expr, $backtrace:expr) => {{
+        use std::backtrace::BacktraceStatus;
+
+        let backtrace = $backtrace;
+
+        match backtrace.status() {
+            BacktraceStatus::Captured => format!("\n[{}]\n{}\n", $prefix, backtrace),
+            BacktraceStatus::Disabled => {
+                "(backtrace disabled, run application with `RUST_BACKTRACE=1` environment variable)"
+                    .to_string()
+            }
+            BacktraceStatus::Unsupported => {
+                "(backtrace unsupported for current platform)".to_string()
+            }
+            _ => "(backtrace status unknown)".to_string(),
+        }
+    }};
+
+    ($prefix:expr) => {
+        $crate::format_backtrace!($prefix, std::backtrace::Backtrace::capture())
+    };
+}
+
+#[cfg(not(debug_assertions))]
+#[macro_export]
+macro_rules! format_backtrace {
+    ($prefix:expr $(, $backtrace:expr)? ) => {
+        String::new()
+    };
+}
+
 pub fn set_gdext_hook<F>(godot_print: F)
 where
     F: Fn() -> bool + Send + Sync + 'static,
@@ -259,11 +296,14 @@ where
 
         let message = format_panic_message(panic_info);
         if godot_print() {
+            // Also prints to stdout/stderr -- do not print twice.
             godot_error!("{message}");
+        } else {
+            eprintln!("{message}");
         }
-        eprintln!("{message}");
-        #[cfg(debug_assertions)]
-        eprintln!("{}", std::backtrace::Backtrace::capture());
+
+        let backtrace = format_backtrace!("panic backtrace");
+        eprintln!("{backtrace}");
         let _ignored_result = std::io::stderr().flush();
     }));
 }
@@ -322,6 +362,7 @@ thread_local! {
 pub fn get_gdext_panic_context() -> Option<String> {
     #[cfg(debug_assertions)]
     return ERROR_CONTEXT_STACK.with(|cell| cell.borrow().get_last());
+
     #[cfg(not(debug_assertions))]
     None
 }
@@ -337,13 +378,18 @@ where
     E: Fn() -> String,
     F: FnOnce() -> R + std::panic::UnwindSafe,
 {
+    #[cfg(not(debug_assertions))]
+    let _ = error_context; // Unused in Release.
+
     #[cfg(debug_assertions)]
     ERROR_CONTEXT_STACK.with(|cell| unsafe {
         // SAFETY: &error_context is valid for lifetime of function, and is removed from LAST_ERROR_CONTEXT before end of function.
         cell.borrow_mut().push_function(&error_context)
     });
+
     let result =
         std::panic::catch_unwind(code).map_err(|payload| extract_panic_message(payload.as_ref()));
+
     #[cfg(debug_assertions)]
     ERROR_CONTEXT_STACK.with(|cell| cell.borrow_mut().pop_function());
     result

godot-core/src/registry/class.rs

@@ -216,9 +216,13 @@ pub fn auto_register_classes(init_level: InitLevel) {
     // but it is much slower and doesn't guarantee that all the dependent classes will be already loaded in most cases.
     register_classes_and_dyn_traits(&mut map, init_level);
 
-    // actually register all the classes
+    // Actually register all the classes.
     for info in map.into_values() {
+        #[cfg(feature = "debug-log")]
+        let class_name = info.class_name;
+
         register_class_raw(info);
+
         out!("Class {class_name} loaded.");
     }
 

godot-core/src/storage/instance_storage.rs

@@ -4,6 +4,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
+
 use godot_ffi as sys;
 use std::cell::Cell;
 use std::ptr;

godot-core/src/storage/mod.rs

@@ -11,4 +11,174 @@ mod multi_threaded;
 #[cfg_attr(feature = "experimental-threads", allow(dead_code))]
 mod single_threaded;
 
+use godot_ffi::out;
 pub use instance_storage::*;
+use std::any::type_name;
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Shared code for submodules
+
+fn bind_failed<T>(err: Box<dyn std::error::Error>, tracker: &DebugBorrowTracker) -> ! {
+    let ty = type_name::<T>();
+
+    eprint!("{tracker}");
+
+    panic!(
+        "Gd<T>::bind() failed, already bound; T = {ty}.\n  \
+        Make sure to use `self.base_mut()` or `self.base()` instead of `self.to_gd()` when possible.\n  \
+        Details: {err}."
+    )
+}
+
+fn bind_mut_failed<T>(err: Box<dyn std::error::Error>, tracker: &DebugBorrowTracker) -> ! {
+    let ty = type_name::<T>();
+
+    eprint!("{tracker}");
+
+    panic!(
+        "Gd<T>::bind_mut() failed, already bound; T = {ty}.\n  \
+        Make sure to use `self.base_mut()` instead of `self.to_gd()` when possible.\n  \
+        Details: {err}."
+    )
+}
+
+fn bug_inaccessible<T>(err: Box<dyn std::error::Error>) -> ! {
+    // We should never hit this, except maybe in extreme cases like having more than `usize::MAX` borrows.
+    let ty = type_name::<T>();
+
+    panic!(
+        "`base_mut()` failed for type T = {ty}.\n  \
+        This is most likely a bug, please report it.\n  \
+        Details: {err}."
+    )
+}
+
+fn log_construct<T>() {
+    out!(
+        "    Storage::construct             <{ty}>",
+        ty = type_name::<T>()
+    );
+}
+
+fn log_inc_ref<T: StorageRefCounted>(storage: &T) {
+    out!(
+        "    Storage::on_inc_ref (rc={rc})     <{ty}> -- {base:?}",
+        rc = T::godot_ref_count(storage),
+        base = storage.base(),
+        ty = type_name::<T>(),
+    );
+}
+
+fn log_dec_ref<T: StorageRefCounted>(storage: &T) {
+    out!(
+        "  | Storage::on_dec_ref (rc={rc})     <{ty}> -- {base:?}",
+        rc = T::godot_ref_count(storage),
+        base = storage.base(),
+        ty = type_name::<T>(),
+    );
+}
+
+fn log_drop<T: StorageRefCounted>(storage: &T) {
+    out!(
+        "    Storage::drop (rc={rc})           <{base:?}>",
+        rc = storage.godot_ref_count(),
+        base = storage.base(),
+    );
+}
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Tracking borrows in Debug mode
+
+#[cfg(debug_assertions)]
+use borrow_info::DebugBorrowTracker;
+
+#[cfg(not(debug_assertions))]
+use borrow_info_noop::DebugBorrowTracker;
+
+#[cfg(debug_assertions)]
+mod borrow_info {
+    use std::backtrace::Backtrace;
+    use std::fmt;
+    use std::sync::Mutex;
+
+    struct TrackedBorrow {
+        backtrace: Backtrace,
+        is_mut: bool,
+    }
+
+    /// Informational-only info about ongoing borrows.
+    pub(super) struct DebugBorrowTracker {
+        // Currently just tracks the last borrow. Could technically track 1 mut or N ref borrows, but would need destructor integration.
+        // Also never clears it when a guard drops, assuming that once a borrow fails, there must be at least one previous borrow conflicting.
+        // Is also not yet integrated with "inaccessible" borrows (reborrow through base_mut).
+        last_borrow: Mutex<Option<TrackedBorrow>>,
+    }
+
+    impl DebugBorrowTracker {
+        pub fn new() -> Self {
+            Self {
+                last_borrow: Mutex::new(None),
+            }
+        }
+
+        // Currently considers RUST_BACKTRACE due to performance reasons; force_capture() can be quite slow.
+        // User is expected to set the env var during debug sessions.
+
+        #[track_caller]
+        pub fn track_ref_borrow(&self) {
+            let mut guard = self.last_borrow.lock().unwrap();
+            *guard = Some(TrackedBorrow {
+                backtrace: Backtrace::capture(),
+                is_mut: false,
+            });
+        }
+
+        #[track_caller]
+        pub fn track_mut_borrow(&self) {
+            let mut guard = self.last_borrow.lock().unwrap();
+            *guard = Some(TrackedBorrow {
+                backtrace: Backtrace::capture(),
+                is_mut: true,
+            });
+        }
+    }
+
+    impl fmt::Display for DebugBorrowTracker {
+        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+            let guard = self.last_borrow.lock().unwrap();
+            if let Some(borrow) = &*guard {
+                let mutability = if borrow.is_mut { "bind_mut" } else { "bind" };
+
+                let prefix = format!("backtrace of previous `{mutability}` borrow");
+                let backtrace = crate::format_backtrace!(prefix, &borrow.backtrace);
+
+                writeln!(f, "{backtrace}")
+            } else {
+                writeln!(f, "no previous borrows tracked.")
+            }
+        }
+    }
+}
+
+#[cfg(not(debug_assertions))]
+mod borrow_info_noop {
+    use std::fmt;
+
+    pub(super) struct DebugBorrowTracker;
+
+    impl DebugBorrowTracker {
+        pub fn new() -> Self {
+            Self
+        }
+
+        pub fn track_ref_borrow(&self) {}
+
+        pub fn track_mut_borrow(&self) {}
+    }
+
+    impl fmt::Display for DebugBorrowTracker {
+        fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
+            Ok(())
+        }
+    }
+}