[mono][interp] Properly handle exceptions thrown by mono_marshal_get_native_wrapper #110232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ution This can end up calling into managed where exceptions can be thrown. Throwing exceptions while method compilation happens in not valid behavior. This follows the same approach from the jit side in mono/mono#20177.
BrzVlad
requested review from
kotlarmilos,
steveisok and
vitek-karas
as code owners
This was referenced Nov 27, 2024
We always optimize managed wrappers. If we are attempting to compile and execute a pinvoke method, we will use the m2n wrapper instead. Make sure we check for this when disabling tiering, since swift interop relies on these wrappers to be always optimized.
kotlarmilos
reviewed
Nov 28, 2024
| @@ -2692,8 +2692,6 @@ static MonoMethod* | |||
| interp_transform_internal_calls (MonoMethod *method, MonoMethod *target_method, MonoMethodSignature *csignature, gboolean is_virtual) | |||
| { | |||
| if (((method->wrapper_type == MONO_WRAPPER_NONE) || (method->wrapper_type == MONO_WRAPPER_DYNAMIC_METHOD)) && target_method != NULL) { | |||
| if (target_method->flags & METHOD_ATTRIBUTE_PINVOKE_IMPL) | |||
| target_method = mono_marshal_get_native_wrapper (target_method, FALSE, FALSE); | |||
| if (!is_virtual && target_method->iflags & METHOD_IMPL_ATTRIBUTE_SYNCHRONIZED) | |||
| target_method = mono_marshal_get_synchronized_wrapper (target_method); | |||
There was a problem hiding this comment.
Can mono_marshal_get_synchronized_wrapper run managed callbacks that can end up throwing an exception as well?
There was a problem hiding this comment.
No. This scenario was happening when looking up for the pinvoke call in libraries, which is not relevant for the synchronized wrapper.
3 tasks
kotlarmilos
approved these changes
Nov 29, 2024
eduardo-vp
pushed a commit
to eduardo-vp/runtime
that referenced
this pull request
Dec 5, 2024
…native_wrapper (dotnet#110232) * [mono][interp] Defer calls to mono_marshal_get_native_wrapper at execution This can end up calling into managed where exceptions can be thrown. Throwing exceptions while method compilation happens in not valid behavior. This follows the same approach from the jit side in mono/mono#20177. * [mono][interp] Check resume state when returning from method compilation * [mono][interp] Fix tiering disable condition We always optimize managed wrappers. If we are attempting to compile and execute a pinvoke method, we will use the m2n wrapper instead. Make sure we check for this when disabling tiering, since swift interop relies on these wrappers to be always optimized.
mikelle-rogers
pushed a commit
to mikelle-rogers/runtime
that referenced
this pull request
Dec 10, 2024
…native_wrapper (dotnet#110232) * [mono][interp] Defer calls to mono_marshal_get_native_wrapper at execution This can end up calling into managed where exceptions can be thrown. Throwing exceptions while method compilation happens in not valid behavior. This follows the same approach from the jit side in mono/mono#20177. * [mono][interp] Check resume state when returning from method compilation * [mono][interp] Fix tiering disable condition We always optimize managed wrappers. If we are attempting to compile and execute a pinvoke method, we will use the m2n wrapper instead. Make sure we check for this when disabling tiering, since swift interop relies on these wrappers to be always optimized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mono_marshal_get_native_wrappercan run some managed callbacks that can end up throwing an exception. This means both that if an exception is thrown we could leak compilation data and we weren't checking for the resume state, leading to executing bogus code when returning. This PR follows the approach JIT from mono/mono#20177 to defer the throwing of exception when the method is actually called and not during compilation and we add the necessary resume state checks.Should fix #101370