Skip to content
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

[mono][wasm] Add interpreter support for calls to UnmanagedCallersOnly EntryPoint without managed delegate reference #95087

Closed
Tracked by #95084
lewing opened this issue Nov 21, 2023 · 10 comments · Fixed by #100288
Assignees
Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono in-pr There is an active PR which will close this issue when it is merged os-browser Browser variant of arch-wasm os-wasi Related to WASI variant of arch-wasm
Milestone

Comments

@lewing
Copy link
Member

lewing commented Nov 21, 2023

#94615 (comment)

#94615 added EntryPoint support to UCO on wasm (it was missing before)

 
        [UnmanagedCallersOnly(EntryPoint = "display_meaning")]
        public static void DisplayMeaningExport(int meaning)
        {
            Console.WriteLine($"The meaning of life is {meaning}");
        }

but there is currently an issue with when invoking the export from unmanaged when there is no managed delegate pointing to it and something akin to the following workaround is required:

   Intptr workaround = (IntPtr)(delegate* unmanaged<int,void>)&display_meaning;  // needs to be in method loaded by interp prior to unmanaged call 

This is because get_native_to_interp must be called before attempting to invoke the unmanaged entry point or the call will fail. Without the call wasm_native_to_interp_ftndescs is never initialized. For the workaround to work the delegate * does not need to be reached/reachable, the interpreter simply needs to resolve the method prior to calling it from unmanaged.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 21, 2023
@lewing lewing added this to the 9.0.0 milestone Nov 21, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 21, 2023
@lewing lewing added the arch-wasm WebAssembly architecture label Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

#94615 (comment)

#94615 added EntryPoint support to UCO

 
        [UnmanagedCallersOnly(EntryPoint = "display_meaning")]
        public static void DisplayMeaningExport(int meaning)
        {
            Console.WriteLine($"The meaning of life is {meaning}");
        }

but there is currently an issue with when invoking the export from unmanaged when there is no managed delegate pointing to it.

   Intptr workaround = (IntPtr)(delegate* unmanaged<int,void>)&display_meaning;  // needs to be in method loaded by interp prior to unmanaged call 
Author: lewing
Assignees: -
Labels:

arch-wasm, area-VM-meta-mono

Milestone: 9.0.0

@lewing lewing added os-wasi Related to WASI variant of arch-wasm os-browser Browser variant of arch-wasm labels Nov 21, 2023
@lewing lewing self-assigned this Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

#94615 (comment)

#94615 added EntryPoint support to UCO

 
        [UnmanagedCallersOnly(EntryPoint = "display_meaning")]
        public static void DisplayMeaningExport(int meaning)
        {
            Console.WriteLine($"The meaning of life is {meaning}");
        }

but there is currently an issue with when invoking the export from unmanaged when there is no managed delegate pointing to it.

   Intptr workaround = (IntPtr)(delegate* unmanaged<int,void>)&display_meaning;  // needs to be in method loaded by interp prior to unmanaged call 

unless get_native_to_interp is called before attempting to invoke the unmanaged entry point the call will fail because the n2m entry slot is not initialized.

Author: lewing
Assignees: lewing
Labels:

arch-wasm, area-Codegen-Interpreter-mono, area-VM-meta-mono, os-wasi, os-browser

Milestone: 9.0.0

@lewing lewing assigned kg and unassigned lewing Nov 21, 2023
@lewing
Copy link
Member Author

lewing commented Nov 27, 2023

@maraf this is essentially the same issue you are seeing with the JSExport for nativeAOT changes, by which I mean for mono we could drastically simplify the current initialization code if we fix the fallback case here

@lewing lewing assigned steveisok and unassigned kg Jan 31, 2024
@lewing
Copy link
Member Author

lewing commented Jan 31, 2024

cc @vargaz @BrzVlad

@lewing
Copy link
Member Author

lewing commented Jan 31, 2024

@steveisok @vitek-karas this is blocking wasi work

@vargaz
Copy link
Contributor

vargaz commented Feb 1, 2024

The generator probably needs to generate some extra code to lookup the MonoMethod*, and call something like mono_jit_compile_method () on it to have the interpreter initialize the pointers.

@vargaz
Copy link
Contributor

vargaz commented Feb 1, 2024

Something like:

MonoMethod *
mono_marshal_get_managed_wrapper (MonoMethod *method, MonoClass *delegate_klass, MonoGCHandle target_handle, MonoError *error);

	MonoError error;
	mono_error_init (&error);
	MonoMethod* m = lookup_dotnet_method("Wasi.Console.Sample", "", "Test", "DisplayMeaningExport", 1);
	MonoMethod *w = mono_marshal_get_managed_wrapper (m, NULL, 0, &error);
	mono_compile_method (w);

Will force the pointers to be initialized. This can be done either on startup at the end of mono_wasm_load_runtime (), or on demand in the generated pinvoke func, i.e.

void display_meaning (int arg0) {
   if (!wasm_native_to_interp_ftndescs [2].func) <INIT>
  ((WasmInterpEntrySig_2)wasm_native_to_interp_ftndescs [2].func) ((int*)&arg0, wasm_native_to_interp_ftndescs [2].arg);
}

@lewing lewing assigned akoeplinger and unassigned steveisok Feb 21, 2024
@knutwannheden
Copy link

@lewing Do you have any more details (or even better an example I can look at) on the mentioned workaround? I tried adding some code like that to the Main method, but that didn't really help.

@lewing lewing assigned mkhamoyan and unassigned akoeplinger Mar 21, 2024
@maraf
Copy link
Member

maraf commented Mar 24, 2024

Do you have any more details (or even better an example I can look at) on the mentioned workaround? I tried adding some code like that to the Main method, but that didn't really help.

@knutwannheden It's something that can't be done in user. I needs to happen in generate C code that works as a trampoline to call managed code

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 26, 2024
@knutwannheden
Copy link

Excellent news! Looking forward to the next release containing this fix ❤️

@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono in-pr There is an active PR which will close this issue when it is merged os-browser Browser variant of arch-wasm os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants