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

Introduce CallConvSuppressGCTransition type to enable suppressing the transition frame for unmanaged function pointer calls #46343

Merged

Conversation

jkoritzinsky
Copy link
Member

Fixes #38134 for CoreCLR and crossgen2. This PR does not provide an implementation for Mono.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/coreclr/vm/corelib.h Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.h Outdated Show resolved Hide resolved
src/coreclr/vm/jitinterface.cpp Show resolved Hide resolved
src/coreclr/vm/siginfo.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubgen.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubgen.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubgen.cpp Outdated Show resolved Hide resolved
@jkoritzinsky
Copy link
Member Author

cc: @BrzVlad for the Mono interpreter failures on the updated test.

@lambdageek
Copy link
Member

lambdageek commented Dec 29, 2020

@jkoritzinsky @BrzVlad @SamMonoRT I created #46451 to track Mono support for CallConvSuppressGCTransition.

Edit: it's not really an interp failure, I think. I think it's the interop/marshaling code - the call to mono_marshal_get_native_wrapper is returning NULL probably.

@BrzVlad
Copy link
Member

BrzVlad commented Dec 30, 2020

@lambdageek The crash happens because interp thinks it is doing a normal managed calli. In order to fix this, we would first need for the runtime to handle the unmanaged call convention (#38480). Setting pinvoke = TRUE for this type of signatures fixes the crash. We aren't trying to generate any m2n wrappers at this point, so further fixes would still be needed on interp side.

@jkoritzinsky jkoritzinsky force-pushed the callconv-suppressgctransition branch from 38a9451 to 15c530e Compare January 4, 2021 22:35
@jkoritzinsky
Copy link
Member Author

@AaronRobinsonMSFT @jkotas @elinor-fung can you take another review pass?

src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
@@ -1384,16 +1387,6 @@ private void findSig(CORINFO_MODULE_STRUCT_* module, uint sigTOK, CORINFO_CONTEX

Get_CORINFO_SIG_INFO(methodSig, sig);

// TODO: Replace this with a public mechanism to mark calli with SuppressGCTransition once it becomes available.
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that the replacement for this is part of this change, but it only affects nativeaot compiler. It is ok - we will fix in in the nativeaot branch.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

src/coreclr/vm/siginfo.cpp Show resolved Hide resolved
src/coreclr/vm/stubgen.cpp Show resolved Hide resolved
src/coreclr/vm/stubgen.cpp Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jan 6, 2021

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jkoritzinsky
Copy link
Member Author

I added the flags back to the cache since it seems that they’re required to disambiguate between some stubs.

@jkoritzinsky jkoritzinsky merged commit 92870e5 into dotnet:master Jan 7, 2021
@jkoritzinsky jkoritzinsky deleted the callconv-suppressgctransition branch January 7, 2021 02:19
@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose a new CallConvSuppressGCTransition so SuppressGCTransition works for function pointers
6 participants