-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix WASM logic for locating correct stubs. #119516
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
The broken scenario was that value types are passed by value if they are <= sizeof(void*), but pass by reference if > sizeof(void*). This needed to be reconciled with the current interpreter ABI that passes values types on the stack.
Tagging subscribers to this area: @mangod9 |
I do not think that the rules are that simple. Take a look at https://godbolt.org/z/8aTqWMEb3 . It shows that |
I think, at least for now, the only additional nuance I should apply is the multi field aspect. I was searching for a nice clear spec, but it looks like chatgpt is about as helpful as can be. |
The next failing issue is the following stack:
The associated IR is below. This appears to be the
|
Managed CALLI needs to do similar dance as regular call for FEATURE_PORTABLE_ENTRYPOINT: If there is no interpreter code attached to the PortableEntryPoint, call DoPrestub and stay in the interpreter loop if the interpreter code shows up. |
@jkotas Thanks. That let me move a bit farther, we're now hitting the following: runtime/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.ActivatorCache.cs Lines 124 to 138 in cecdae6
The particular issue here is runtime/src/coreclr/vm/reflectioninvocation.cpp Line 1637 in cecdae6
|
Oops. Looks like we went down the following path: runtime/src/coreclr/vm/reflectioninvocation.cpp Lines 1656 to 1658 in cecdae6
Looks like we will need to update |
Great progress. I think this is the spec/convention you were looking for https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md#function-arguments-and-return-values |
Yes, looks like it. |
Co-authored-by: Jan Kotas <[email protected]>
signature: void *(valuetype System.Runtime.CompilerServices.QCallTypeHandle,valuetype System.TypeNameFormatFlags,valuetype System.Runtime.CompilerServices.StringHandleOnStack)
signature: valuetype System.RuntimeMethodHandleInternal *(valuetype System.Runtime.CompilerServices.QCallModule,int32,native int*,int32,native int*,int32)
Just curious - where are all these calls coming from? |
Good question. I have got a bit carried away and didn't look closely. We are getting to the first one from
|
This is because we are trying to load |
It is not interesting to work on fixing DllNotFoundException throwing path at this point. You will eventually hit exception handling that will quite a bit of work to bring up that's outside the scope of this PR. Try to fix whatever is causing DllNotFoundException to be thrown instead. |
Yes, I will check whether we already need |
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 fixes WASM logic for correctly locating stubs when handling value types in the interpreter ABI. The issue was reconciling how value types are passed: by value if they are <= sizeof(void*)
, but by reference if > sizeof(void*)
, with the current interpreter ABI that passes value types on the stack.
Key changes:
- Enhanced argument conversion logic to support indirect argument passing for large value types
- Added new thunk functions for handling indirect argument signatures
- Refactored helper function resolution to improve code organization
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/coreclr/vm/wasm/helpers.cpp | Added new thunk functions for indirect arguments and enhanced argument type conversion logic to handle value types based on size and field count |
src/coreclr/vm/jitinterface.cpp | Refactored helper function resolution by extracting common logic into getHelperFtnStatic and renaming parameter for clarity |
src/coreclr/vm/interpexec.cpp | Added portable entry point handling for WASM to properly route calls between interpreter and native code |
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.
LGTM
@radekdoulik @pavelsavara Feel free to merge this if it looks good to you as well.
The broken scenario was that value types are passed by value if they are
<= sizeof(void*)
, but pass by reference if> sizeof(void*)
. This needed to be reconciled with the current interpreter ABI that passes values types on the stack.