-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Conversation
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.
src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs
Outdated
Show resolved
Hide resolved
The mono changes look ok. Does this mean that the EntryPoint property is mandatory on qcalls ? Should the runtime code verify that ? |
src/libraries/Native/AnyOS/System.IO.Compression.Native/entrypoints.c
Outdated
Show resolved
Hide resolved
That should not be the case. If |
@vargaz, @elinor-fung is correct. I've added the |
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/libraries/Native/Unix/System.Globalization.Native/entrypoints.c
Outdated
Show resolved
Hide resolved
Co-authored-by: Elinor Fung <elfung@microsoft.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.