[coop][interp] Fix GC transitions for thunk invoke wrappers #81773
Conversation
The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)
lambdageek
requested review from
BrzVlad,
vargaz and
kotlarmilos
as code owners
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos Issue DetailsThe code that recognizes GC transition icalls in the interpreter was only assuming that it will see which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms) Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct. |
My point is, there is no attach happening. Any user code that calls the thunk returned by
Yea I guess I could just change |
It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup. In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the |
This reverts commit 583ad5a.
Only assert in debug builds
Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.
|
@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter |
|
update oops, wrong PR |
ghost
locked as resolved and limited conversation to collaborators
The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced/mono_threads_exit_gc_safe_region_unbalancedwhich are used by managed-to-native wrappers.In most cases for native-to-managed wrappers the marshaller emits
mono_threads_attach_coop/mono_threads_detach_coopand those are handled elsewhere by setting theneeds_thread_attachflag on theInterpMethod.However when the
mono_marshal_get_thunk_invoke_wrapperAPI is used, we emit a thunk invoke wrapper which usesmono_threads_enter_gc_unsafe_region_unbalanced/mono_threads_exit_gc_unsafe_region_unbalancedChange the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.
As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.
Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)