Panic handling: thread safety; set hook once and not repeatedly

[KevinThierauf]

Work-in-progress change to how gdext handles panics. This PR is mostly complete, but there's one or two things I don't understand and wanted to get feedback on. Mostly, it comes down to handle_varcall_panic and handle_ptrcall_panic; the existing implementation of handle_panic_with_print provides a print parameter, which allows handle_varcall_panic and handle_ptrcall_panic to suppress printing on error. This PR removes the parameter, and prints anyways in the panic hook. I don't understand the intention behind suppressing the print call for the two aforementioned functions, and I don't want to modify their behavior until I understand the reasoning. I marked this PR as a draft because of this.
Otherwise, this PR should be mostly straightforward. Instead of calling set_hook for all calls to handle_panic_with_print, the set_hook function is set only on initialization. This should both make panic handling thread safe, and also allow users to set their own panic hook without conflicting with gdext.
There is one unsafe block added. It's used to optimize debug information (so error_context remains unevaluated, unless it is needed). Specifically, it casts &dyn Fn() -> String to &'static dyn Fn(), so that the value can be (temporarily) pushed onto a thread_local vec, operating as a stack. This 'static reference will be removed by a drop guard before it is invalidated, which prevents any UB access.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1037

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1037

[Bromeon]

Thank you! 🙂

---

This PR removes the parameter, and prints anyways in the panic hook. I don't understand the intention behind suppressing the print call for the two aforementioned functions, and I don't want to modify their behavior until I understand the reasoning. I marked this PR as a draft because of this.

This needs to stay, it's used to make the integration tests less spammy, see here. Eventually we might do this via cfg, but for now I don't want to make the crate setup more complex.

Thanks for double-checking, it's appreciated! 👍

---

There is one unsafe block added. It's used to optimize debug information (so error_context remains evaluated, unless it is needed). Specifically, it casts &dyn Fn() -> String to &'static dyn Fn(), so that the value can be (temporarily) pushed onto a thread_local vec, operating as a stack. This 'static reference will be removed by a drop guard before it is invalidated, which prevents any UB access.

This sounds a bit shady. This part adds a great deal of complexity with thread_local! and unsafe, so could you elaborate what we are optimizing exactly? What if we didn't do this optimization?

---

Another thing you removed is the #[cfg(debug_assertions)]. Quite some discussion went into the status quo, see #889 and #923. I'm inclined to keep this distinction -- panics in Release should be as fast as possible, while information density is secondary.

[KevinThierauf]

This needs to stay, it's used to make the integration tests less spammy, see here.

Oh I see. The linked source temporarily replaces the panic hook to suppress the print, which should suppress printing now (previously printing was handled outside of the panic hook, but now that it's back in, this should be enough on its own).

There is one unsafe block added. It's used to optimize debug information (so error_context remains evaluated, unless it is needed). Specifically, it casts &dyn Fn() -> String to &'static dyn Fn(), so that the value can be (temporarily) pushed onto a thread_local vec, operating as a stack. This 'static reference will be removed by a drop guard before it is invalidated, which prevents any UB access.

This sounds a bit shady. This part adds a great deal of complexity with thread_local! and unsafe, so could you elaborate what we are optimizing exactly? What if we didn't do this optimization?

It is a little shady, but it's ultimately used to avoid evaluating the error_context unless it is explicitly requested. Previously, handle_panic_with_print (now handle_panic) requested a impl Fn() -> impl Display, which could be called to request the error context. Because the print occurred inside handle_panic_with_print, the lifetime was acceptable. However, error handling has been moved back to the panic hook, which means (in order to access the context) it needs to be moved into some sort of global.
The easiest (and safest) way of doing this is to eagerly evaluate the error_context to a String (and passing that to a global), but that would mean evaluating error_context even when no panic occurs. This instead is avoided by passing the error_context function to a thread-local global, where it can be evaluated if and only if it is requested with get_gdext_panic_context (e.g. if a panic occurs).
It should be completely valid; error_context (with a local lifetime) is converted to 'static and pushed onto a global, but removed from that global immediately before the lifetime is invalidated. In other words, the 'static lifetime is needed to place the function in a global, but the lifetime is respected.

thread-local is used to avoid conflicts with multiple threads. ERROR_CONTEXT is a stack stored on global memory, allowing any pushed error_context values to apply to any nested function calls, but should only apply to nested function calls. Function calls on separate threads should not inherit the error context (doubly important with the experimental-threads feature).

Another thing you removed is the #[cfg(debug_assertions)]. Quite some discussion went into the status quo, see #889 and #923. I'm inclined to keep this distinction -- panics in Release should be as fast as possible, while information density is secondary.

I'll double check the performance impact of removing #[cfg(debug_assertions)]. I think this PR should be faster in debug builds now that set_hook is only applied once, removing the debug information mutex, etc. (which is probably what ate up the cycles). Specifically, debug_assertions builds should have about the same code complexity in this PR as non debug_assertions builds had previously, which is where the cfgs went.
Removing the location information now would just mean removing:

    let prefix = if let Some(location) = _location {
        format!(
            "panic {}:{}",
            location.file(),
            location.line(),
        )
    } else {
        "panic".to_owned()
    };

Which I don't think would affect performance notably, but I'll double check the performance with this commit/without it to see if there's any difference on release builds to see if there's something else I'm missing.

[KevinThierauf]

I ran some quick tests to check performance between the latest gdext master and this commit. I used the latest godot master, and built both debug and release template builds. Running on windows 10.

	Master	#1037
GDScript (Debug)	130ms	130ms
GDScript (Release)	74ms	74ms
Rust (Debug)	235ms	110ms
Rust (Release)	15ms	21ms

I ran a test similar to the previously mentioned test, displaying the number of ms required to run 1mil noop calls.

So there's definitely something going on here. I expected a speedup in debug builds, but nowhere near 2x; the performance in debug builds now beats gdscript. Conversely, I wasn't really expecting much change at all in release, but this commit runs about 33% slower. I'll do some digging to see if I can figure out what's happening here.

Edit: Unsurprisingly, the slowdown has to due with storing the error context. Removing the error context (so that handle_panic just calls catch_unwind) leads to the same performance as the current master.
Using std::cell::Cell<&'static dyn Fn() -> String> reduces performance by about 1ms, which is pretty much just the cost of the thread_local storage.
Using std::cell::RefCell<&'static dyn Fn() -> String> (no Vec stack, but otherwise identical to #1037) performance was around 3ms slower.
In both cases, the error_context is only set once, instead of being pushed/popped (which explains why using RefCell without the Vec stack is about half as much as with the Vec stack; pushing/popping to the Vec doesn't really impact performance much, but the Vec stack implementation borrows it twice -- once to add, once to remove -- as opposed to the reference, which only borrows once).
Unfortunately, using a Cell<...> instead of a RefCell<Vec<...>> means that nested calls won't have their error_context values respected, so I don't think it can be used.
It's worth noting that 1ms is 1ms/1mil calls. In other words, each ms is really 1ns/call. Additionally, I wasn't expecting have to dig into this, so the tests aren't as robust as they might otherwise be; all timings are estimations (+/- a ms or two).

In summary: the easiest solution would be to mark the error_context behind the debug_assertions flag. This would result in identical performance to the current gdext master. Release panics would now include location information of the panic, but the error_context would only be available on debug builds.
Alternatively, release builds could eat the performance of storing error_context, which would result in a roughly 33% slowdown for noop calls (+5ns/function call). This shouldn't make a difference unless a user makes a truly exorbitant number of calls across the FFI boundary; about 200,000 calls would eat up an extra ms (on my machine -- YMMV).

[Bromeon]

Thanks a lot for the detailed answer and for the measurements! 👍

---

It should be completely valid; error_context (with a local lifetime) is converted to 'static and pushed onto a global, but removed from that global immediately before the lifetime is invalidated. In other words, the 'static lifetime is needed to place the function in a global, but the lifetime is respected.

Ah I see now, so it's similar to the scoped-tls crate. This makes sense; it also seems like extending the lifetime to 'static is sound, as long as the reference isn't accessed past its original lifetime (I checked with Miri).

That said, I wonder if it wouldn't be clearer if instead of transmuting the lifetime to &'static, you would insert a raw pointer into the stack, and then simply have a // SAFETY: statement on unsafe access. This will make it less tempting to accidentally reuse the reference (e.g. store it) in the hook. What do you think?

---

Unfortunately, using a Cell<...> instead of a RefCell<Vec<...>> means that nested calls won't have their error_context values respected, so I don't think it can be used.

Could you elaborate "nested calls" with a small (pseudo-)code scenario? Isn't panicking while a panic hook is processing already a problem?

---

In summary: the easiest solution would be to mark the error_context behind the debug_assertions flag. This would result in identical performance to the current gdext master. Release panics would now include location information of the panic, but the error_context would only be available on debug builds.

I do think this might be the better trade-off. There are many places where we give away performance of FFI calls due to subtle little interactions in the library, and this also reflects badly in library benchmarks. Eventually I'd like to make this configurable in a very coarse way (setting global flags during library init), but then it would be easier to already have some case differentiation.

[lilizoey]

Thanks a lot for the detailed answer and for the measurements! 👍

It should be completely valid; error_context (with a local lifetime) is converted to 'static and pushed onto a global, but removed from that global immediately before the lifetime is invalidated. In other words, the 'static lifetime is needed to place the function in a global, but the lifetime is respected.

Ah I see now, so it's similar to the scoped-tls crate. This makes sense; it also seems like extending the lifetime to 'static is sound, as long as the reference isn't accessed past its original lifetime (I checked with Miri).

I can confirm that this is true. lifetimes are effectively just a safety invariant on references, meaning that breaking them wont actually cause any undefined behavior. so it is sound to create references with too large lifetimes, provided nothing ever accesses them after their true lifetime ends.

That said, I wonder if it wouldn't be clearer if instead of transmuting the lifetime to &'static, you would insert a raw pointer into the stack, and then simply have a // SAFETY: statement on unsafe access. This will make it less tempting to accidentally reuse the reference (e.g. store it) in the hook. What do you think?

I would agree that it'd be nicer to use a type that clearly indicates the unsafety here. but i dont think a raw pointer will work? because they're not Sync and so can't be used in statics. AtomicPtr or a newtype wrapper should work however.

[KevinThierauf]

That said, I wonder if it wouldn't be clearer if instead of transmuting the lifetime to &'static, you would insert a raw pointer into the stack, and then simply have a // SAFETY: statement on unsafe access. This will make it less tempting to accidentally reuse the reference (e.g. store it) in the hook. What do you think?

Yeah, using pointers makes things more clear. I added a type ScopedFunctionStack, which might be overkill, but I think it makes it clearer. I can inline it if if makes things too verbose though.

Unfortunately, using a Cell<...> instead of a RefCell<Vec<...>> means that nested calls won't have their error_context values respected, so I don't think it can be used.

Could you elaborate "nested calls" with a small (pseudo-)code scenario? Isn't panicking while a panic hook is processing already a problem?

Sorry, I meant nested calls to handle_panic. I'm not super concerned about optimizing though, now that it's locked behind debug_assertions.

I do think this might be the better trade-off. There are many places where we give away performance of FFI calls due to subtle little interactions in the library, and this also reflects badly in library benchmarks. Eventually I'd like to make this configurable in a very coarse way (setting global flags during library init), but then it would be easier to already have some case differentiation.

Fair enough.

I would agree that it'd be nicer to use a type that clearly indicates the unsafety here. but i dont think a raw pointer will work? because they're not Sync and so can't be used in statics. AtomicPtr or a newtype wrapper should work however.

One of the nifty benefits of thread_local is that it doesn't require Sync, so it should be okay. I did add a wrapper type anyways, and I think it makes things clearer, but it also make things a bit more verbose, so I'm amenable to inline it if it's too much of a problem.

[KevinThierauf]

So I think I understand why the linux tests are failing. Broadly speaking, it's because HOT_RELOADING_ENABLED in linux_reload_workaround.rs is set as a OnceCell -- which can only be set once -- meaning if is_hot_reload_enabled is called before enable_hot_reload, enable_hot_reload immediately panics. Reading the comments in linux_reload_workaround.rs indicate that is_hot_reload_enabled related to TLS storage:

Turns glibc's TLS destructor register function, __cxa_thread_atexit_impl, into a no-op if hot reloading is enabled.

Some stacktrace indicates that modifying ERROR_CONTEXT_STACK (the thread_local from this PR) leads to is_hot_reload_enabled being called, which makes enable_hot_reload fail.
Kind of a weird bug, but simple enough fix -- changing

static HOT_RELOADING_ENABLED: OnceLock<bool> = OnceLock::new();

to

static HOT_RELOADING_ENABLED: AtomicBool = AtomicBool::new(false);

(plus the get/set calls) fixes everything.
Should I fix it in a new PR? Or would it be better to include it here?

[Bromeon]

Sorry, I meant nested calls to handle_panic.

Yes, but when do they happen? Did you encounter this?
Should we not try to avoid it?

---

Some stacktrace indicates that modifying ERROR_CONTEXT_STACK (the thread_local from this PR) leads to is_hot_reload_enabled being called, which makes enable_hot_reload fail.
Kind of a weird bug, but simple enough fix

This happens here, in the thread_atexit hook.

While AtomicBool may make the issue go away, I'd rather be sure that the hot-reload-enabled flag is initialized early enough so that it's not an issue. This should be done very early, on startup. Is the whole panic handling not only set up later, or why the interference here?

Either way, would be good to have a separate PR to discuss this 🙂

[Bromeon]

If you use raw pointers, lifetimes are thrown away, so the extension with assume_static_lifetime is no longer needed. Just insert the raw pointer directly.

Furthermore, which function is unsafe depends a bit on how we lay out the responsibilities. I'd spontaneously have said that get_last() should be unsafe since it's the one introducing UB when dereferencing a dangling pointer, but on the other hand there's no real invariant the caller can uphold inside the panic hook; they rely on everything being done correctly in handle_panic.

As such, this probably makes sense?

@lilizoey opinions on this?

[KevinThierauf]

If you use raw pointers, lifetimes are thrown away, so the extension with assume_static_lifetime is no longer needed. Just insert the raw pointer directly.

I'm not a hundred percent certain why, but inserting the raw pointer without the call to assume_static_lifetime results in a compiler error:

error: lifetime may not live long enough
   --> godot-core\src\private.rs:296:29
    |
287 |     unsafe fn push_function(&mut self, function: &dyn Fn() -> String) {
    |                                                  - let's call the lifetime of this reference `'1`
...
296 |         self.functions.push(function as *const dyn Fn() -> String);
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cast requires that `'1` must outlive `'static`
error: could not compile `godot-core` (lib) due to 1 previous error

[KevinThierauf]

I think it has to do with the lifetime of the function trait itself, but I'm not certain.

[lilizoey]

Furthermore, which function is unsafe depends a bit on how we lay out the responsibilities. I'd spontaneously have said that get_last() should be unsafe since it's the one introducing UB when dereferencing a dangling pointer, but on the other hand there's no real invariant the caller can uphold inside the panic hook; they rely on everything being done correctly in handle_panic.

As such, this probably makes sense?

@lilizoey opinions on this?

Honestly it's a bit of a tossup, at least one of push_function and get_last have to be unsafe for sure. I think this works as is though. The api isn't the easiest to use safely but we also dont use this anywhere else, so it's probably fine.

I think it has to do with the lifetime of the function trait itself, but I'm not certain.

Yeah, you can do

function as (*const dyn Fn() -> String) as (*const (dyn Fn() -> String + 'static))

[Bromeon]

This is still pending 🙂

First, let's not use as cast to go from reference to pointer, there's a dedicated function for it. Unfortunately, we can't use ptr::cast() for the second conversion, as that implicitly needs Sized for some reason.

Using intermediate variables for clarity, this then becomes:

    let function = std::ptr::from_ref(function);
    let function = function as *const (dyn Fn() -> String + 'static);

---

Sidenote: as _ casts are a rusty crowbar which is very dangerous, let's write types explicitly in this context, not with _. It's ironic that mem::transmute got a lint to specify types explicitly, but pointer bending at will is totally not seen as a problem, not even in clippy. (I'm not saying it should be unsafe, of course; a lint would be nice though).

[KevinThierauf]

Sorry, I meant nested calls to handle_panic.

Yes, but when do they happen? Did you encounter this? Should we not try to avoid it?

I was assuming (maybe incorrectly?) that handle_panic could be called recursively, such as in the event that a callable invokes another callable. It should work either way.

Altogether, this PR should be pretty much done, pending some kind of fix for the interference with TLS causing the linux tests to fail (#1040).

[Bromeon]

I noticed you changed code style in several places and deleted some comments documenting behavior, but the previous code had often a reason to be. So I added some comments asking to revert such cases.

Sorry for being nitpicky, but panic handling is a particularly central and important part of the codebase 😉

Once we find a solution for the hot-reload flag, I'll also try to run it locally to test.

[Bromeon]

This is still pending 🙂

First, let's not use as cast to go from reference to pointer, there's a dedicated function for it. Unfortunately, we can't use ptr::cast() for the second conversion, as that implicitly needs Sized for some reason.

Using intermediate variables for clarity, this then becomes:

    let function = std::ptr::from_ref(function);
    let function = function as *const (dyn Fn() -> String + 'static);

---

Sidenote: as _ casts are a rusty crowbar which is very dangerous, let's write types explicitly in this context, not with _. It's ironic that mem::transmute got a lint to specify types explicitly, but pointer bending at will is totally not seen as a problem, not even in clippy. (I'm not saying it should be unsafe, of course; a lint would be nice though).

[KevinThierauf]

For:

let function = std::ptr::from_ref(function);
let function = function as *const (dyn Fn() -> String + 'static);

Clippy complains that the second cast is unnecessary:

warning: casting raw pointers to the same type and constness is unnecessary (`*const dyn std::ops::Fn() -> std::string::String` -> `*const dyn std::ops::Fn() -> std::string::String`)
   --> godot-core\src\private.rs:297:24
    |
297 |         let function = function as *const (dyn Fn() -> String + 'static);
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `function`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
    = note: `#[warn(clippy::unnecessary_cast)]` on by default

... and yet, without it, it doesn't acknowledge the function is being 'static. I added an allow, which I hope is okay. On the bright side, it allows us to get rid of the awkward assume_static_lifetime.