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.

Sign up for GitHub

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

Jump to bottom

Add missing exception checkpoint #41098

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Add missing exception checkpoint #41098

merged 2 commits into from
Aug 21, 2020

Conversation

fanyang-mono
Copy link
Member

Fixes #38826

@ghost
Copy link

ghost commented Aug 20, 2020

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

@fanyang-mono fanyang-mono requested a review from SamMonoRT August 20, 2020 16:18
@monojenkins monojenkins mentioned this pull request Aug 20, 2020
Merged
@SamMonoRT SamMonoRT requested a review from CoffeeFlux August 20, 2020 16:54
@SamMonoRT
Copy link
Member

Enable the tests (remove from issues.target file) to make sure change actually fixes the affected ones in #38826

@fanyang-mono
Copy link
Member Author

Enable the tests (remove from issues.target file) to make sure change actually fixes the affected ones in #38826

Oops, good catch!

CoffeeFlux
CoffeeFlux reviewed Aug 20, 2020
View reviewed changes
src/mono/mono/mini/interp/interp.c
@@ -3847,6 +3847,7 @@ interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs
frame->state.ip = ip + 6;
ves_pinvoke_method (csignature, (MonoFuncV)code, context, frame, &retval, save_last_error, cache, sp);

EXCEPTION_CHECKPOINT_GC_UNSAFE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we back in GC safe here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Followed the same pattern we did for MINT_CALLI_NAT_FAST.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why we're in GC unsafe there, though I guess if it throws we must be? @BrzVlad do you know what's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the story behind it mono/mono#17016. You could also read the issue it mentioned in this PR. In short, usually the interpreter operates in GC Unsafe state.

SamMonoRT
SamMonoRT approved these changes Aug 21, 2020
View reviewed changes
Copy link
Member

@SamMonoRT SamMonoRT left a comment

Choose a reason for hiding this comment

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

Please check the logs to verify if all the Interop/NativeLibrary/API/NativeLibraryTests were included and passed prior to merging the PR.

@fanyang-mono
Copy link
Member Author

I saw this from helix log:

Interop.NativeLibrary.XUnitWrapper Total: 2, Errors: 0, Failed: 0, Skipped: 0, Time: 0.406s

There are in total 4 tests under Interop/NativeLibrary. Two of them are still disabled. From the log, there are 2 tests were run. Thus, the tests being enabled with this PR was run and passed.

@SamMonoRT
Copy link
Member

Should the two disabled tests pass with your change, or are they another issue ?

@CoffeeFlux
Copy link
Contributor

CoffeeFlux commented Aug 21, 2020 •
edited
Loading

ResolveUnmanagedDllTests and CallbackStressTest_TargetUnix are disabled globally for Mono; this PR looks fine.
It's possible they're now passing with the recent pinvoke fixes, but I can handle that separately.

@fanyang-mono fanyang-mono merged commit 1748156 into dotnet:master Aug 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@fanyang-mono fanyang-mono deleted the no_exception branch May 27, 2021 14:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CoffeeFlux CoffeeFlux CoffeeFlux left review comments

@SamMonoRT SamMonoRT SamMonoRT approved these changes

@BrzVlad BrzVlad BrzVlad approved these changes

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-Codegen-Interpreter-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Mono Interp] CoreCLR tests failed with Mono interpreter: Interop/NativeLibrary/API/NativeLibraryTests
5 participants
@fanyang-mono @SamMonoRT @CoffeeFlux @BrzVlad @Dotnet-GitSync-Bot