[mono][wasm] Use [UnmanagedCallersOnly] in GetDelegateForFunctionPoin… #89334
Conversation
…terTests so it works on wasm. Fixes dotnet#39187.
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue Details…terTests so it works on wasm. Fixes #39187.
|
|
Looks like the tests only pass in non-aot mode if compiled with /p:WasmBuildNative=true. |
That makes some sense given how we use the table generator for this attribute. We might be able to add a check to a platform analyzer, but we should definitely document it. |
jeffschwMSFT
added
area-Interop-mono
and removed
area-System.Runtime.InteropServices
labels
|
@vargaz what's the plan here? |
|
Not sure, this test will not pass in non-aot mode on wasm even with these changes. |
|
cc @kg |
|
We could run the table generator and check if any new entries were added over the default and emit an error if WasmBuildNative is not enabled. |
|
|
||
| private static void Method(string s) { } | ||
| [UnmanagedCallersOnly] |
There was a problem hiding this comment.
NativeAOT uses UnmanagedFunctionPointerAttribute on the delegate definitions to make delegate stubs available to the AOT compiler when they can't figure it out directly from arguments to GetFunctionPointerForDelegate or GetDelegateForFunctionPointer
|
I'm not sure how I feel about this test change. We're narrowing our test coverage to no longer cover "what happens when you pass a managed type across the boundary". I guess we probably have enough comprehensive test coverage for p/invoke that it's fine to do so? |
|
The string->int change is needed because UnmanagedCallersOnly requires blittable types. |
|
I guess my concern is that it feels like we're replacing test A with a new test B, when we could leave A disabled and add the new test alongside it to avoid reducing our test coverage. Then if we fix string support later we can turn the existing test on. |
|
This test is supposed to test GetDelegateForFunctionPointer, not pinvoke in general. |
|
/azp run runtime |
|
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This will not work in non-aot mode even with these changes, closing. |
…terTests so it works on wasm.
Fixes #39187.