-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[wasm][coreclr] callhelpers generator, pinvoke part #123090
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
base: main
Are you sure you want to change the base?
Conversation
Generate src/coreclr/vm/wasm/callhelpers-pinvoke.cpp and use it instead of hardcoded native libs entrypoints.c
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.
Pull request overview
This PR implements code generation for WASM/CoreCLR PInvoke tables instead of using hardcoded native library entrypoints. The generator scans managed assemblies and creates C++ files that map library names to their native function entry points.
Changes:
- Modified the PInvoke table generator to split generation into separate files for PInvoke and reverse PInvoke (callbacks)
- Added callhelpers-pinvoke.cpp generation that provides
callhelpers_pinvoke_overridefunction for resolving native library imports - Updated build system to conditionally compile the generated PInvoke table when
GEN_PINVOKE=1 - Removed
libzmodule dependency (now covered by System.IO.Compression.Native) and added System.Native.Browser module - Modified native library build to use empty.c stub when generator is enabled
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tasks/WasmAppBuilder/coreclr/PInvokeTableGenerator.cs | Refactored Generate method to create separate PInvoke and reverse PInvoke files; updated C++ output format to use minipal entrypoints; added QCall filtering |
| src/tasks/WasmAppBuilder/coreclr/ManagedToNativeGenerator.cs | Added ReversePInvokeOutputPath parameter to support separate reverse PInvoke generation |
| src/tasks/WasmAppBuilder/WasmAppBuilder.csproj | Updated module list (added System.Native.Browser, removed libz) and configured output paths for generated files |
| src/native/libs/System.Native.Browser/CMakeLists.txt | Added empty.c as source when GEN_PINVOKE is enabled to provide stub implementation |
| src/native/libs/CMakeLists.txt | Removed CORERUN_LIBS_ONLY condition for System.IO.Compression.Native to ensure it's always built |
| src/coreclr/vm/wasm/callhelpers-reverse.cpp | Regenerated with updated hash values for reverse thunk entries |
| src/coreclr/vm/wasm/callhelpers-pinvoke.cpp | New generated file containing PInvoke tables and callhelpers_pinvoke_override implementation |
| src/coreclr/vm/pinvokeoverride.cpp | Excluded GlobalizationResolveDllImport for WASM targets (now handled by generated code) |
| src/coreclr/vm/CMakeLists.txt | Conditionally includes callhelpers-pinvoke.cpp when GEN_PINVOKE is set |
| src/coreclr/hosts/corerun/wasm/pinvoke_override.cpp | Simplified to declare and use callhelpers_pinvoke_override from generated code |
| src/coreclr/hosts/corerun/CMakeLists.txt | Added System.IO.Compression.Native-Static and System.Native.Browser-Static dependencies |
| src/coreclr/CMakeLists.txt | Enabled GEN_PINVOKE flag for Browser/WASM builds |
Comments suppressed due to low confidence (1)
src/coreclr/hosts/corerun/CMakeLists.txt:67
- System.Native.Browser-Static is already linked at line 57. This duplicate link specification is redundant and should be removed to avoid unnecessary repetition.
target_link_libraries(corerun PRIVATE
System.Native.Browser-Static)
Co-authored-by: Copilot <[email protected]>
| void * SystemNative_GetDefaultSearchOrderPseudoHandle (); | ||
| int32_t SystemNative_GetErrNo (); | ||
| uint32_t SystemNative_GetFileSystemType (void *); | ||
| int32_t SystemNative_GetIPv4Address (void *, int32_t, void *); |
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.
SystemNative_GetIPv4Address is suspect in the browser. It suggests that you generated the file with System.Net.Sockets.dll in the set. It would be good to use browser flavor of the runtime pack. Ideally with unreachable code trimmed.
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| #include <string.h> |
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.
| #define _In_z_ | ||
| #define _In_ |
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.
| #define _In_ | |
| #include <sal.h> |
Please give this a try.
| #ifdef TARGET_WASM | ||
| return nullptr; | ||
| #else // !TARGET_WASM | ||
| return DefaultResolveDllImport(libraryName, entrypointName); | ||
| #endif // TARGET_WASM |
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.
| #ifdef TARGET_WASM | |
| return nullptr; | |
| #else // !TARGET_WASM | |
| return DefaultResolveDllImport(libraryName, entrypointName); | |
| #endif // TARGET_WASM | |
| return DefaultResolveDllImport(libraryName, entrypointName); |
| #ifndef TARGET_WASM | ||
| // here we handle PInvokes whose implementation is always statically linked (even in .so/.dll case) | ||
| static const void* DefaultResolveDllImport(const char* libraryName, const char* entrypointName) | ||
| { | ||
| if (strcmp(libraryName, GLOBALIZATION_DLL_NAME) == 0) | ||
| { | ||
| return GlobalizationResolveDllImport(entrypointName); | ||
| } | ||
|
|
||
| return nullptr; | ||
| } | ||
| #endif // TARGET_WASM |
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.
| // here we handle PInvokes whose implementation is always statically linked (even in .so/.dll case) | |
| static const void* DefaultResolveDllImport(const char* libraryName, const char* entrypointName) | |
| { | |
| #ifndef TARGET_WASM | |
| if (strcmp(libraryName, GLOBALIZATION_DLL_NAME) == 0) | |
| { | |
| return GlobalizationResolveDllImport(entrypointName); | |
| } | |
| #endif // TARGET_WASM | |
| return nullptr; | |
| } |
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 don't fully understand why we can't reconcile this entry point with the pinvoke_override.cpp though.
|
|
||
| if (NOT GEN_PINVOKE) | ||
| if (GEN_PINVOKE) | ||
| set (BROWSER_SOURCES ${BROWSER_SOURCES} empty.c) |
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.
Why is an empty file needed?
| { 2638856333, 4239234100, { &MD_System_Private_CoreLib_Internal_Runtime_InteropServices_ComActivator_RegisterClassForTypeInternal_I32_RetI32, (void*)&Call_System_Private_CoreLib_Internal_Runtime_InteropServices_ComActivator_RegisterClassForTypeInternal_I32_RetI32 } } /* alternate key source: RegisterClassForTypeInternal#1:System.Private.CoreLib:Internal.Runtime.InteropServices:ComActivator */, | ||
| { 2638830480, 167179540, { &MD_System_Private_CoreLib_System_Threading_TimerQueue_TimerHandler_Void_RetVoid, (void*)&Call_System_Private_CoreLib_System_Threading_TimerQueue_TimerHandler_Void_RetVoid } } /* alternate key source: TimerHandler#0:System.Private.CoreLib:System.Threading:TimerQueue */, | ||
| { 2638856332, 2150642223, { &MD_System_Private_CoreLib_Internal_Runtime_InteropServices_ComActivator_UnregisterClassForTypeInternal_I32_RetI32, (void*)&Call_System_Private_CoreLib_Internal_Runtime_InteropServices_ComActivator_UnregisterClassForTypeInternal_I32_RetI32 } } /* alternate key source: UnregisterClassForTypeInternal#1:System.Private.CoreLib:Internal.Runtime.InteropServices:ComActivator */ | ||
| { 2644319180, 3863938719, { &MD_System_Private_CoreLib_System_GC__RegisterNoGCRegionCallback_g__Callback_7C_72_0_I32_RetVoid, (void*)&Call_System_Private_CoreLib_System_GC__RegisterNoGCRegionCallback_g__Callback_7C_72_0_I32_RetVoid } } /* alternate key source: <RegisterNoGCRegionCallback>g__Callback|72_0#1:System.Private.CoreLib:System:GC */, |
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.
What caused this hash to change ?
Generate src/coreclr/vm/wasm/callhelpers-pinvoke.cpp and use it instead of hardcoded native libs entrypoints.c