Skip to content

Add UnmanagedCallConvAttribute #52869

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
Merged
elinor-fung merged 9 commits into from
May 21, 2021
Merged

Add UnmanagedCallConvAttribute #52869

elinor-fung merged 9 commits into from
May 21, 2021

Conversation

@elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented May 17, 2021 •
edited by AaronRobinsonMSFT
Loading

Fixes #51156

  • Add UnmanagedCallConvAttribute
  • Use UnmanagedCallConv if the calling convention is not specified or specified as winapi on pinvokeimpl metadata
    • Also used for suppressing GC transition, since CallConvSuppressGCTransition can be specified
    • Check attribute in mono - cc @CoffeeFlux @lambdageek
    • Check attribute in crossgen2 - cc @trylek
  • Add tests
  • Not handled in crossgen
    • Similar to UnmanagedCallersOnly - since we don't include the support to read a type array, make crossgen bail if the method is marked UnmanagedCallConv

This does not implement checking of UnmanagedCallConv for delegates.

cc @AaronRobinsonMSFT @jkoritzinsky

@ghost
Copy link

ghost commented May 17, 2021

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.

@elinor-fung elinor-fung force-pushed the unmanagedCallConv branch 2 times, most recently from 58d494e to 26651ea Compare May 17, 2021 23:50
@jkotas
Copy link
Member

jkotas commented May 18, 2021

This also does not implement checking of UnmanagedCallConv for delegates

I would not bother with implementing the advanced calling conventions for delegates. I think we should make everybody who needs to do interop with function pointers to use function pointers.

@elinor-fung elinor-fung marked this pull request as ready for review May 18, 2021 18:26
lambdageek
lambdageek reviewed May 18, 2021
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

It would be great if the mono code could avoid managed allocation to decode the custom attribute. That would make it usable for AOT compilation. If that turns out not to work, we should still take the code, disabled, and do a follow-up fix to mono_reflection_create_custom_attr_data_args_noalloc to make it work.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 18, 2021

I would not bother with implementing the advanced calling conventions for delegates.

I am okay with this also. I would like to see us error out though if it is set on a Delegate - unless someone can argue it being a no-op is more appropriate.

@jkotas
Copy link
Member

jkotas commented May 18, 2021

I would like to see us error out though if it is set on a Delegate

If you try to use this on a Delegate, the compiler will produce an error thanks to AttributeTargets.Method.

lambdageek
lambdageek reviewed May 19, 2021
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

mono bits lgtm

jkoritzinsky
jkoritzinsky reviewed May 20, 2021
View reviewed changes
@elinor-fung
Copy link
Member Author

elinor-fung commented May 21, 2021

All the tests passed - failure was from uploading results (azure.devops.exceptions.AzureDevOpsClientRequestError: Operation returned an invalid status code of 503)

@elinor-fung elinor-fung merged commit d8fec85 into dotnet:main May 21, 2021
@elinor-fung elinor-fung deleted the unmanagedCallConv branch May 21, 2021 06:07
This was referenced May 21, 2021
Open
Merged
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Jun 7, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lambdageek lambdageek lambdageek left review comments

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@CoffeeFlux CoffeeFlux Awaiting requested review from CoffeeFlux

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@vargaz vargaz Awaiting requested review from vargaz

Assignees

No one assigned

Labels

area-Interop-coreclr new-api-needs-documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extensible Calling Conventions for native callee functions

5 participants

@elinor-fung @jkotas @AaronRobinsonMSFT @lambdageek @jkoritzinsky