fix #21369, exception state overwritten by caught internal errors #21434
Conversation
ararslan
added
kind:bugfix
src/gc.c
Outdated
| @@ -108,7 +108,7 @@ static void run_finalizer(jl_ptls_t ptls, jl_value_t *o, jl_value_t *ff) | |||
| JL_TRY { | |||
| size_t last_age = jl_get_ptls_states()->world_age; | |||
| jl_get_ptls_states()->world_age = jl_world_counter; | |||
| jl_apply(args, 2); | |||
| jl_apply_with_saved_exception_state(args, 2, 0); | |||
| jl_get_ptls_states()->world_age = last_age; | |||
| } | |||
| JL_CATCH { | |||
There was a problem hiding this comment.
We should either remove this or keep it working. (Printing a warning if an unexpected error happens in a finalizer is probably still somewhat useful.)
There was a problem hiding this comment.
Actually didn't realized that this is what the 0 is for...
There was a problem hiding this comment.
Though in that case, does the copying of backtrace actually do anything at all? If an error is thrown it'll just hit this catch and the exception and backtraces will still be overwritten? Seems that the saving should move to the caller of this function, outside the loop.
There was a problem hiding this comment.
Yes, it still does something, in case a finalizer catches and hides an exception. However you're right that saving the state inside this loop is inefficient. C is just not good for this kind of programming :(
There was a problem hiding this comment.
in case a finalizer catches and hides an exception.
Ah, yeah.
C is just not good for this kind of programming :(
Agreed. Luckily, there's only one caller so it should be sufficient to add the saving logic to jl_gc_run_finalizers_in_lilst after releasing the finalizer lock.
JeffBezanson
force-pushed
the
jb/fix21369
branch
from
ba8bf6a
to
849c358
Compare
|
Updated. |
|
@vtjnash There's an interesting error on 64-bit linux on travis here where my change to static_eval apparently tries to call something in world 0. Why would this happen only on 64-bit travis/linux? Works locally. |
|
I'm not sure why only x86-64 exercises this code path, but it is correct in observing that previously this function did not exercise any dynamic dispatch code, and with the change, it now is attempting to do so. |
|
How could the current world get set to 0? Is there any code that does that? |
|
It's the default world. It can sometimes get triggered when you try to run |
JeffBezanson
force-pushed
the
jb/fix21369
branch
from
849c358
to
4f256ee
Compare
No description provided.