Skip to content
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

[mono] Pass the pending exception correctly from interp_runtime_invok… #72126

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Jul 13, 2022

…e ().

Fixes #71838.

@vargaz
Copy link
Contributor Author

vargaz commented Jul 13, 2022

/azp run runtime-wasm

@ghost
Copy link

ghost commented Jul 13, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

…e ().

Fixes #71838.

Author: vargaz
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz added arch-wasm WebAssembly architecture and removed area-System.Text.Json labels Jul 13, 2022
@ghost
Copy link

ghost commented Jul 13, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

…e ().

Fixes #71838.

Author: vargaz
Assignees: vargaz
Labels:

arch-wasm

Milestone: -

@vargaz
Copy link
Contributor Author

vargaz commented Jul 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Jul 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz marked this pull request as ready for review July 15, 2022 02:02
@vargaz vargaz requested a review from BrzVlad as a code owner July 15, 2022 02:02
@vargaz vargaz requested a review from lambdageek July 15, 2022 16:55
@@ -2119,13 +2119,13 @@ interp_runtime_invoke (MonoMethod *method, void *obj, void **params, MonoObject

context->stack_pointer = (guchar*)sp;

check_pending_unwind (context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also need to change interp_entry ?

Copy link
Member

@BrzVlad BrzVlad Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_pending_unwind seems dubious to me. It throws a C++ exception, but this is only caught in llvmonly code. This code probably needs some refactoring.

I think the intended behavior for check_pending_unwind should be

if (mono_aot_mode == MONO_AOT_MODE_LLVMONLY_INTERP && context->has_resume_state)
    mono_llvm_cpp_throw_exception ();

Additionally, when exiting the interpreter (returning from interp_entry, interp_entry_from_trampoline, interp_runtime_invoke) we probably should assert !context->has_resume_state, because if we have resume state here it means we are in non-llvmonly mode where we resume directly to the ip of the handling code. When returning to EH (interp_run_finally ...) we can probably assert that we have context->handler_frame set if we have resume state, since otherwise we should have resumed already)

Maybe we should have 2 check_pending_unwind methods, one for returning to compiled/native and the other for returning to EH since their behavior is quite different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !context->handler_frame check can only be true in mixed mode.

@lewing lewing merged commit c895d95 into dotnet:main Jul 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] STJ.Serialization.Tests failing with System.NotSupportedException with AOT
4 participants