Fix gc rooting in exception throwing #28770
Conversation
Most paths to `throw_internal` assume that `e` may be unrooted. However, the static analyzer complains that a transition from gc-safe to gc-unsafe (`jl_gc_unsafe_enter`) is a safepoint, so `e` may have been collected there before it gets assigned to `exception_in_transit` (which is a root). Switch the order of operations to fix that.
There was a problem hiding this comment.
The change looks valid though I don't think there's actually a bug here before.
The goal of the unsafe enter is to make sure the GC state is always unsafe (managed) in the catch frame, it should not affect the liveness of e. In fact, if the state was GC safe before entering the function (allowed but probably not used yet), e had better be rooted somewhere or another thread can free it. If it wasn't rooted, the state should have been unsafe and the unsafe_enter should be a no-op.
In any case, the change is still fine. Maybe worth add a comment if you want.
|
Unrelatedly, can we rename these from safe/unsafe to unmanaged/managed? I always mess up which is which. |
That was the original name until #14190 (comment) |
|
I propose enter/leave managed |
|
Fine with me too... I don't like naming things and I hate renaming things so I'm not going to vote or make a decision on this................. |
|
Backport candidate? |
Most paths to
throw_internalassume thatemay be unrooted. However, the static analyzer complains that a transition from gc-safe to gc-unsafe (jl_gc_unsafe_enter) is a safepoint, soemay have been collected there before it gets assigned toexception_in_transit(which is a root). Switch the order of operations to fix that.