From 67ebc571c062de5a571f64283a67c418d875ad28 Mon Sep 17 00:00:00 2001 From: Kevin Date: Sun, 9 Feb 2025 07:22:41 -0500 Subject: [PATCH 1/9] linux_reload_workaround changes --- godot-ffi/src/linux_reload_workaround.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/godot-ffi/src/linux_reload_workaround.rs b/godot-ffi/src/linux_reload_workaround.rs index 12e543ccb..187d0dd5b 100644 --- a/godot-ffi/src/linux_reload_workaround.rs +++ b/godot-ffi/src/linux_reload_workaround.rs @@ -13,12 +13,13 @@ // See: https://fasterthanli.me/articles/so-you-want-to-live-reload-rust#what-can-prevent-dlclose-from-unloading-a-library use std::ffi::c_void; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::OnceLock; pub type ThreadAtexitFn = unsafe extern "C" fn(*mut c_void, *mut c_void, *mut c_void); static SYSTEM_THREAD_ATEXIT: OnceLock> = OnceLock::new(); -static HOT_RELOADING_ENABLED: OnceLock = OnceLock::new(); +static HOT_RELOADING_ENABLED: AtomicBool = AtomicBool::new(false); fn system_thread_atexit() -> &'static Option { SYSTEM_THREAD_ATEXIT.get_or_init(|| unsafe { @@ -29,14 +30,12 @@ fn system_thread_atexit() -> &'static Option { pub fn enable_hot_reload() { // If hot reloading is enabled then we should properly unload the library, so this will only be called once. - HOT_RELOADING_ENABLED - .set(true) - .expect("hot reloading should only be set once") + HOT_RELOADING_ENABLED.store(true, Ordering::SeqCst); } pub fn disable_hot_reload() { // If hot reloading is disabled then we may call this method multiple times. - _ = HOT_RELOADING_ENABLED.set(false) + HOT_RELOADING_ENABLED.store(false, Ordering::SeqCst); } pub fn default_set_hot_reload() { @@ -54,7 +53,7 @@ fn is_hot_reload_enabled() -> bool { // destructors exist for good reasons. // This is needed for situations like unit-tests, where we may create TLS-destructors without explicitly calling any of the methods // that set hot reloading to be enabled or disabled. - *HOT_RELOADING_ENABLED.get_or_init(|| false) + HOT_RELOADING_ENABLED.load(Ordering::SeqCst) } /// Turns glibc's TLS destructor register function, `__cxa_thread_atexit_impl`, From 277379ea5a7c19108da0956b995a5433c805ce08 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 13 Feb 2025 23:30:22 -0500 Subject: [PATCH 2/9] revert changes, is_hot_reload_enabled panic instead of initialize --- godot-ffi/src/linux_reload_workaround.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/godot-ffi/src/linux_reload_workaround.rs b/godot-ffi/src/linux_reload_workaround.rs index 187d0dd5b..3521e930b 100644 --- a/godot-ffi/src/linux_reload_workaround.rs +++ b/godot-ffi/src/linux_reload_workaround.rs @@ -13,13 +13,12 @@ // See: https://fasterthanli.me/articles/so-you-want-to-live-reload-rust#what-can-prevent-dlclose-from-unloading-a-library use std::ffi::c_void; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::OnceLock; pub type ThreadAtexitFn = unsafe extern "C" fn(*mut c_void, *mut c_void, *mut c_void); static SYSTEM_THREAD_ATEXIT: OnceLock> = OnceLock::new(); -static HOT_RELOADING_ENABLED: AtomicBool = AtomicBool::new(false); +static HOT_RELOADING_ENABLED: OnceLock = OnceLock::new(); fn system_thread_atexit() -> &'static Option { SYSTEM_THREAD_ATEXIT.get_or_init(|| unsafe { @@ -30,12 +29,14 @@ fn system_thread_atexit() -> &'static Option { pub fn enable_hot_reload() { // If hot reloading is enabled then we should properly unload the library, so this will only be called once. - HOT_RELOADING_ENABLED.store(true, Ordering::SeqCst); + HOT_RELOADING_ENABLED + .set(true) + .expect("hot reloading should only be set once") } pub fn disable_hot_reload() { // If hot reloading is disabled then we may call this method multiple times. - HOT_RELOADING_ENABLED.store(false, Ordering::SeqCst); + _ = HOT_RELOADING_ENABLED.set(false); } pub fn default_set_hot_reload() { @@ -53,7 +54,7 @@ fn is_hot_reload_enabled() -> bool { // destructors exist for good reasons. // This is needed for situations like unit-tests, where we may create TLS-destructors without explicitly calling any of the methods // that set hot reloading to be enabled or disabled. - HOT_RELOADING_ENABLED.load(Ordering::SeqCst) + *HOT_RELOADING_ENABLED.get().expect("hot reloading status has not yet been marked as enabled/disabled") } /// Turns glibc's TLS destructor register function, `__cxa_thread_atexit_impl`, From 706772f2150a97b750b747e170792e33e7bc4db1 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 13 Feb 2025 23:38:19 -0500 Subject: [PATCH 3/9] change override_hot_reload() to const --- godot-core/src/init/mod.rs | 41 +++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index b268dda94..167bfcc98 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -23,16 +23,17 @@ pub unsafe fn __gdext_load_library( library: sys::GDExtensionClassLibraryPtr, init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { + // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to + // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this could cause the default + // behavior to kick in and disable hot reloading. + #[cfg(target_os = "linux")] + match E::override_hot_reload { + None => sys::linux_reload_workaround::default_set_hot_reload(), + Some(true) => sys::linux_reload_workaround::enable_hot_reload(), + Some(false) => sys::linux_reload_workaround::disable_hot_reload(), + } + let init_code = || { - // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to - // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this could cause the default - // behavior to kick in and disable hot reloading. - #[cfg(target_os = "linux")] - match E::override_hot_reload() { - None => sys::linux_reload_workaround::default_set_hot_reload(), - Some(true) => sys::linux_reload_workaround::enable_hot_reload(), - Some(false) => sys::linux_reload_workaround::disable_hot_reload(), - } let tool_only_in_editor = match E::editor_run_behavior() { EditorRunBehavior::ToolClassesOnly => true, @@ -234,6 +235,14 @@ fn gdext_on_level_deinit(level: InitLevel) { // FIXME intra-doc link #[doc(alias = "entry_symbol", alias = "entry_point")] pub unsafe trait ExtensionLibrary { + /// Whether to enable hot reloading of this library. Return `None` to use the default behavior. + /// + /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no + /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds. + /// + /// By default, this is enabled for debug builds and disabled for release builds. + const OVERRIDE_HOT_RELOAD: Option = None; + /// Determines if and how an extension's code is run in the editor. fn editor_run_behavior() -> EditorRunBehavior { EditorRunBehavior::ToolClassesOnly @@ -261,20 +270,6 @@ pub unsafe trait ExtensionLibrary { fn on_level_deinit(level: InitLevel) { // Nothing by default. } - - /// Whether to enable hot reloading of this library. Return `None` to use the default behavior. - /// - /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no - /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds. - /// - /// By default, this is enabled for debug builds and disabled for release builds. - /// - /// Note that this is only checked *once* upon initializing the library. Changing this from `true` to `false` will be picked up as the - /// library is then fully reloaded upon hot-reloading, however changing it from `false` to `true` is almost certainly not going to work - /// unless hot-reloading is already working regardless of this setting. - fn override_hot_reload() -> Option { - None - } } /// Determines if and how an extension's code is run in the editor. From eeb0c2658d471b785d952343a8154976468a8ee2 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 13 Feb 2025 23:41:32 -0500 Subject: [PATCH 4/9] fix --- godot-core/src/init/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 167bfcc98..40e9dffcf 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -27,7 +27,7 @@ pub unsafe fn __gdext_load_library( // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this could cause the default // behavior to kick in and disable hot reloading. #[cfg(target_os = "linux")] - match E::override_hot_reload { + match E::OVERRIDE_HOT_RELOAD { None => sys::linux_reload_workaround::default_set_hot_reload(), Some(true) => sys::linux_reload_workaround::enable_hot_reload(), Some(false) => sys::linux_reload_workaround::disable_hot_reload(), From 8d6767f433178671496933a3fd711abd6bec3861 Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 14 Feb 2025 00:48:17 -0500 Subject: [PATCH 5/9] cargo fmt --- godot-core/src/init/mod.rs | 6 ++---- godot-ffi/src/linux_reload_workaround.rs | 4 +++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 40e9dffcf..c5ca97761 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -24,8 +24,7 @@ pub unsafe fn __gdext_load_library( init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to - // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this could cause the default - // behavior to kick in and disable hot reloading. + // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this will panic. #[cfg(target_os = "linux")] match E::OVERRIDE_HOT_RELOAD { None => sys::linux_reload_workaround::default_set_hot_reload(), @@ -34,7 +33,6 @@ pub unsafe fn __gdext_load_library( } let init_code = || { - let tool_only_in_editor = match E::editor_run_behavior() { EditorRunBehavior::ToolClassesOnly => true, EditorRunBehavior::AllClasses => false, @@ -235,7 +233,7 @@ fn gdext_on_level_deinit(level: InitLevel) { // FIXME intra-doc link #[doc(alias = "entry_symbol", alias = "entry_point")] pub unsafe trait ExtensionLibrary { - /// Whether to enable hot reloading of this library. Return `None` to use the default behavior. + /// Whether to enable hot reloading of this library. Set `None` to use the default behavior. /// /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds. diff --git a/godot-ffi/src/linux_reload_workaround.rs b/godot-ffi/src/linux_reload_workaround.rs index 3521e930b..4a7a55d3a 100644 --- a/godot-ffi/src/linux_reload_workaround.rs +++ b/godot-ffi/src/linux_reload_workaround.rs @@ -54,7 +54,9 @@ fn is_hot_reload_enabled() -> bool { // destructors exist for good reasons. // This is needed for situations like unit-tests, where we may create TLS-destructors without explicitly calling any of the methods // that set hot reloading to be enabled or disabled. - *HOT_RELOADING_ENABLED.get().expect("hot reloading status has not yet been marked as enabled/disabled") + *HOT_RELOADING_ENABLED + .get() + .expect("hot reloading status has not yet been marked as enabled/disabled") } /// Turns glibc's TLS destructor register function, `__cxa_thread_atexit_impl`, From a445ee21f453f7127200e129e3f3524624f1c012 Mon Sep 17 00:00:00 2001 From: Kevin Thierauf Date: Mon, 17 Feb 2025 03:05:28 -0500 Subject: [PATCH 6/9] changes --- godot-ffi/src/linux_reload_workaround.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/godot-ffi/src/linux_reload_workaround.rs b/godot-ffi/src/linux_reload_workaround.rs index 4a7a55d3a..12e543ccb 100644 --- a/godot-ffi/src/linux_reload_workaround.rs +++ b/godot-ffi/src/linux_reload_workaround.rs @@ -36,7 +36,7 @@ pub fn enable_hot_reload() { pub fn disable_hot_reload() { // If hot reloading is disabled then we may call this method multiple times. - _ = HOT_RELOADING_ENABLED.set(false); + _ = HOT_RELOADING_ENABLED.set(false) } pub fn default_set_hot_reload() { @@ -54,9 +54,7 @@ fn is_hot_reload_enabled() -> bool { // destructors exist for good reasons. // This is needed for situations like unit-tests, where we may create TLS-destructors without explicitly calling any of the methods // that set hot reloading to be enabled or disabled. - *HOT_RELOADING_ENABLED - .get() - .expect("hot reloading status has not yet been marked as enabled/disabled") + *HOT_RELOADING_ENABLED.get_or_init(|| false) } /// Turns glibc's TLS destructor register function, `__cxa_thread_atexit_impl`, From c1ab323aaba4d5f5642f4b6d906639af5c830d64 Mon Sep 17 00:00:00 2001 From: Kevin Thierauf Date: Mon, 17 Feb 2025 03:10:07 -0500 Subject: [PATCH 7/9] revert changes --- godot-core/src/init/mod.rs | 41 ++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 49bd6adad..94cfaeb27 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -26,16 +26,17 @@ pub unsafe fn __gdext_load_library( library: sys::GDExtensionClassLibraryPtr, init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { - // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to - // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this will panic. - #[cfg(target_os = "linux")] - match E::OVERRIDE_HOT_RELOAD { - None => sys::linux_reload_workaround::default_set_hot_reload(), - Some(true) => sys::linux_reload_workaround::enable_hot_reload(), - Some(false) => sys::linux_reload_workaround::disable_hot_reload(), - } - let init_code = || { + // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to + // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise, this could cause the default + // behavior to kick in and disable hot reloading. + #[cfg(target_os = "linux")] + match E::override_hot_reload() { + None => sys::linux_reload_workaround::default_set_hot_reload(), + Some(true) => sys::linux_reload_workaround::enable_hot_reload(), + Some(false) => sys::linux_reload_workaround::disable_hot_reload(), + } + let tool_only_in_editor = match E::editor_run_behavior() { EditorRunBehavior::ToolClassesOnly => true, EditorRunBehavior::AllClasses => false, @@ -236,14 +237,6 @@ fn gdext_on_level_deinit(level: InitLevel) { // FIXME intra-doc link #[doc(alias = "entry_symbol", alias = "entry_point")] pub unsafe trait ExtensionLibrary { - /// Whether to enable hot reloading of this library. Set `None` to use the default behavior. - /// - /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no - /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds. - /// - /// By default, this is enabled for debug builds and disabled for release builds. - const OVERRIDE_HOT_RELOAD: Option = None; - /// Determines if and how an extension's code is run in the editor. fn editor_run_behavior() -> EditorRunBehavior { EditorRunBehavior::ToolClassesOnly @@ -271,6 +264,20 @@ pub unsafe trait ExtensionLibrary { fn on_level_deinit(level: InitLevel) { // Nothing by default. } + + /// Whether to enable hot reloading of this library. Return `None` to use the default behavior. + /// + /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no + /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds. + /// + /// By default, this is enabled for debug builds and disabled for release builds. + /// + /// Note that this is only checked *once* upon initializing the library. Changing this from `true` to `false` will be picked up as the + /// library is then fully reloaded upon hot-reloading, however changing it from `false` to `true` is almost certainly not going to work + /// unless hot-reloading is already working regardless of this setting. + fn override_hot_reload() -> Option { + None + } } /// Determines if and how an extension's code is run in the editor. From 78833b3591c22f8feb3c758f92edde0807f08774 Mon Sep 17 00:00:00 2001 From: Kevin Thierauf Date: Mon, 17 Feb 2025 03:13:46 -0500 Subject: [PATCH 8/9] switch init to use std::panic::catch_unwind instead of handle_panic --- godot-core/src/init/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 94cfaeb27..8ab9ec6ea 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -68,8 +68,7 @@ pub unsafe fn __gdext_load_library( success as u8 }; - let ctx = || "error when loading GDExtension library"; - let is_success = crate::private::handle_panic(ctx, init_code); + let is_success = std::panic::catch_unwind(init_code); is_success.unwrap_or(0) } From ac61c5722383b1df60c557348c4e36f529559cef Mon Sep 17 00:00:00 2001 From: Kevin Thierauf Date: Mon, 17 Feb 2025 03:16:41 -0500 Subject: [PATCH 9/9] added comment --- godot-core/src/init/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 8ab9ec6ea..90c19c410 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -68,6 +68,10 @@ pub unsafe fn __gdext_load_library( success as u8 }; + // Use std::panic::catch_unwind instead of handle_panic: handle_panic uses TLS, which + // calls `thread_atexit` on linux, which sets the hot reloading flag on linux. + // Using std::panic::catch_unwind avoids this, although we lose out on context information + // for debugging. let is_success = std::panic::catch_unwind(init_code); is_success.unwrap_or(0)