-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[interp] avoid exception checkpoint when in thread is in GC Safe state #16955
Conversation
mono/mini/interp/interp.c
Outdated
@@ -3206,6 +3207,7 @@ interp_exec_method_full (InterpFrame *frame, ThreadContext *context, FrameClause | |||
GSList *finally_ips = NULL; | |||
const guint16 *ip = NULL; | |||
stackval *sp; | |||
gboolean interp_in_gc_safe_state = FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
benchmarking showed that it's very beneficial to have lower stack usage. Maybe we can have this somewhere else. Like in the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I was "afraid" of such a comment 😉
ThreadContext
wouldn't work as you can have several managed-to-native
frames on the stack, right? Other ideas?
mono/mini/interp/interp.c
Outdated
MONO_REQ_GC_SAFE_MODE | ||
sp = do_icall_wrapper (frame, NULL, MINT_ICALL_PP_V, sp, (gpointer) mono_threads_exit_gc_safe_region_unbalanced, FALSE); | ||
MONO_REQ_GC_UNSAFE_MODE | ||
interp_in_gc_safe_state = FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this stuff work when an exception is thrown and these frames are completely unwound without resuming here ? (this is also available for jit) Or if the native code calls back into the interpreter, will we still be in gc safe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think managed-to-native
frames are supposed to be completely unwound when the exception is thrown from native code. There is an interaction between XI runtime and mono runtime: Let's say native code (ObjC code) is throwing an exception. On a managed<>native barrier the XI runtime will wrap the ObjC exception into a managed exception, use mono_runtime_set_pending_exception
and then we would resume execution in the managed-to-native
wrapper. The thread interruption check in the wrapper would pick up that exception from there and continues with exception handling in Mono; at this point we have transitioned to GC Unsafe already.
Another scenario would be when we have:
<managed #2>
<native>
<managed #1>
And something throws an exception in <managed #2>
, our EH transitions exception handling on the native<>managed barrier via the MONO_FIRST_PASS_CALLBACK_TO_NATIVE
mechanism to the XI runtime. After that, it's the same path as above.
Is there some other scenario you have in mind?
(Note: There is a missing GC transition in this function, https://github.com/xamarin/xamarin-macios/pull/7036/files#diff-7063c4ba2c90067ddc44fc8a7d1d865cR2364 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it would work on ios because we have that callback, but in the general scenario it wouldn't. If we have m1->n->m2 and we throw from m2 while having a catch for that exception inside m1, then we would never have updated interp_in_gc_safe_state for exit and future exception throwing inside the method would be broken. This scenario likely can't happen since these are only emitted for the special m2n wrappers, but it doesn't feel like good design to me, having the true thread state on one hand and trying to emulate it with this per frame variable. If we are going to check whether we are in gc safe or unsafe code when doing exception checkpoint, can't we just check the real state ? (getting rid of the hackish per frame variable and avoiding any bugs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pff, that makes the patch so much simpler 😄
updated the PR!
Usually the interpreter operates in GC Unsafe state. There is an exception however, and that is when it executes a `managed-to-native` wrapper in cooperative (or hybrid) suspend. The wrapper does essentially: 1. enter GC Safe state via `mono_threads_enter_gc_safe_region_unbalanced` 2. call native function 3. exit GC state via `mono_threads_exit_gc_safe_region_unbalanced` Usually upon return of a native function call, the interpreter would check for thread interruption and, if applicable, throw an exception. That, however, would mess with the GC state as the interpreter operates in GC Safe state at this moment. Note that ignoring thread interruption at that point shouldn't be a problem, because after 3. the `managed-to-native` wrapper will check for thread interruption anyway.
954b848
to
90b1296
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinvokes are not the fastest of operations, but it wouldn't hurt to move the unsafe_mode fetching behind interruption requested flag (which is supposed to be the fast path). EXCEPTION_CHECKPOINT_UNSAFE something
@BrzVlad sounds good, will do in a follow-up fix on |
@monojenkins squash |
@monojenkins backport 2019-08 |
@lewurm backporting to 2019-08 failed, the patch results in conflicts:
Please backport manually! |
Cannot squash because the following required status checks are not successful:
|
@monojenkins build Windows i386 |
mono/mono#16955) [interp] avoid exception checkpoint when in thread is in GC Safe state Usually the interpreter operates in GC Unsafe state. There is an exception however, and that is when it executes a `managed-to-native` wrapper in cooperative (or hybrid) suspend. The wrapper does essentially: 1. enter GC Safe state via `mono_threads_enter_gc_safe_region_unbalanced` 2. call native function 3. exit GC state via `mono_threads_exit_gc_safe_region_unbalanced` Usually upon return of a native function call, the interpreter would check for thread interruption and, if applicable, throw an exception. That, however, would mess with the GC state as the interpreter operates in GC Safe state at this moment. Note that ignoring thread interruption at that point shouldn't be a problem, because after 3. the `managed-to-native` wrapper will check for thread interruption anyway. Contributes to mono/mono#16819 Commit migrated from mono/mono@e271591
…mono#17016) [interp] always require GC Unsafe mode in exception checkpoint Follow-up for mono/mono#16955 /cc @BrzVlad Commit migrated from mono/mono@c81b6f5
Usually the interpreter operates in GC Unsafe state. There is an exception however, and that is when it executes a
managed-to-native
wrapper in cooperative (or hybrid) suspend.The wrapper does essentially:
mono_threads_enter_gc_safe_region_unbalanced
mono_threads_exit_gc_safe_region_unbalanced
Usually upon return of a native function call, the interpreter would check for thread interruption and, if applicable, throw an exception. That, however, would mess with the GC state as the interpreter operates in GC Safe state at this moment.
Note that ignoring thread interruption at that point shouldn't be a problem, because after 3. the
managed-to-native
wrapper will check for thread interruption anyway.Contributes to #16819