Skip to content

Make QCalls respect the EntryPoint property #60992

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

Merged
merged 14 commits into from
Nov 2, 2021

Conversation

jkoritzinsky
Copy link
Member

Fixes #59823

Move QCalls to use a similar registration mechanism as the libraries that are bundled in the single file host. All QCalls are now C-style functions and the runtimes now respect the EntryPoint property.

This will enable Mono AOT to more easily resolve QCalls if Mono decides to use them (hopefully this will make using QCalls in Mono more attractive).

Additionally, by making QCalls respect the EntryPoint property, we will be able to move all QCalls to use the DllImportGenerator even for non-blittable QCalls.

Fixes dotnet#59823

Move QCalls to use a similar registration mechanism as the libraries that are bundled in the single file host. All QCalls are now C-style functions and the runtimes now respect the EntryPoint property.

This will enable Mono AOT to more easily resolve QCalls if Mono decides to use them (hopefully this will make using QCalls in Mono more attractive).

Additionally, by making QCalls respect the EntryPoint property, we will be able to move all QCalls to use the DllImportGenerator even for non-blittable QCalls.
@vargaz
Copy link
Contributor

vargaz commented Oct 29, 2021

The mono changes look ok. Does this mean that the EntryPoint property is mandatory on qcalls ? Should the runtime code verify that ?

@elinor-fung
Copy link
Member

Does this mean that the EntryPoint property is mandatory on qcalls ?

That should not be the case. If EntryPoint is not set, the method name is used. I expect @jkoritzinsky added the prefixes for existing QCalls to have reasonable differentiation on the native side, so EntryPoint is set on them since the method name is not the desired entry point.

@jkoritzinsky
Copy link
Member Author

@vargaz, @elinor-fung is correct. I've added the EntryPoint property where the managed and native QCall method names differ. We have a few that are identical, so there were no changes for those needed on the managed side.

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.

Thanks!

DllImportEntry(AssemblyNative_TraceSatelliteSubdirectoryPathProbed)
DllImportEntry(AssemblyNative_GetAssemblyCount)
DllImportEntry(AssemblyNative_GetEntryAssembly)
DllImportEntry(AssemblyNative_GetExecutingAssembly)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to keep the Native suffix on a few of the prefixes (AssemblyNative, ThreadNative, MarshalNative) instead of leaving it off like most of the others?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason. I just did most of this change with a mechanical find-replace, so I didn't want to go back and update these manually since some of these classes (mainly AssemblyNative) are used by multiple managed types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave these names as-is for now. We can clean this up in a future PR.

Co-authored-by: Elinor Fung <elfung@microsoft.com>
jkoritzinsky and others added 3 commits November 2, 2021 10:14
Co-authored-by: Elinor Fung <elfung@microsoft.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@jkoritzinsky jkoritzinsky merged commit 24e7a4a into dotnet:main Nov 2, 2021
@jkoritzinsky jkoritzinsky deleted the qcall-entrypoint branch November 2, 2021 23:37
@@ -82,7 +84,7 @@ Do not replicate the comments into your actual QCall implementation. This is for
class Foo
{
// All QCalls should have the following DllImport attribute
Copy link
Member

Choose a reason for hiding this comment

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

it sounds like CharSet and EntryPoint are mandatory. would it be more accurate to say:

- // All QCalls should have the following DllImport attribute
+ // All QCalls should have the following DllImport attribute (with or without `EntryPoint`)
or simply:
+ // An example of QCall DllImport

Copy link
Member Author

Choose a reason for hiding this comment

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

CharSet and EntryPoint aren't mandatory, but the majority of cases will require at least one of them. We can update the docs/examples if necessary, but I think the guidance is good as a starting place.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 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.

Make QCalls respect EntryPoint
8 participants