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.

Sign up for GitHub

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

Jump to bottom

Disable qcalls on wasm. Treat them as normal pinvokes. #43798

Merged
merged 3 commits into from
Oct 28, 2020
Merged

Disable qcalls on wasm. Treat them as normal pinvokes. #43798

merged 3 commits into from
Oct 28, 2020

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Oct 24, 2020

This is needed because they are stored in a separate table, so they cannot be linked out the same
way as pinvokes/icalls.

@monojenkins monojenkins mentioned this pull request Oct 24, 2020
Merged
marek-safar
marek-safar reviewed Oct 26, 2020
View reviewed changes
src/mono/wasm/wasm.proj
@@ -34,6 +34,7 @@
<ItemGroup>
<WasmPInvokeModule Include="libSystem.Native" />
<WasmPInvokeModule Include="libSystem.IO.Compression.Native" />
<WasmPInvokeModule Include="QCall" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because QCalls are implemented on the managed side as pinvokes with to the module 'QCall'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to handle that inside runtime?

Copy link
Member

@lambdageek lambdageek Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to handle that inside runtime?

We want to make it possible to remove the native code of pinvokes/qcalls that are not called anywhere in managed. If the runtime has an array of qcall functions, the native linker won't be able to drop them

marek-safar
marek-safar reviewed Oct 26, 2020
View reviewed changes
tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs
@@ -154,6 +153,12 @@ private string GenPInvokeDecl(PInvoke pinvoke)
{
var sb = new StringBuilder();
var method = pinvoke.Method;
if (method.Name == "EnumCalendarInfo") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a patch which adds a lot more of these so this won't work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just intended as a temporary hack until that MetadataLoadContext bug is fixed though, so it might be fine for now? Unless you think that patch is landing soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That patch landed yesterday so this does not work anymore

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch NotSupportedException and map to int 😬

Copy link
Member

@lewing lewing Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like all the delegate* in that patch are in assemblies we don't support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018: The "PInvokeTableGenerator" task failed unexpectedly.
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018: System.Exception: The return type 'Interop+BOOL' of pinvoke callback method 'Interop+BOOL EnumCalendarInfoCallback(System.Char*, System.UInt32, System.IntPtr, System.Void*)' needs to be blittable.
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at PInvokeTableGenerator.Error(String msg) in /Users/lewing/Source/runtime/tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 313
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at PInvokeTableGenerator.EmitNativeToInterp(StreamWriter w, List`1 callbacks) in /Users/lewing/Source/runtime/tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 196
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at PInvokeTableGenerator.GenPInvokeTable(String[] pinvokeModules, String[] assemblies) in /Users/lewing/Source/runtime/tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 53
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at PInvokeTableGenerator.Execute() in /Users/lewing/Source/runtime/tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 28
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
/Users/lewing/Source/runtime/src/mono/wasm/wasm.proj(48,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask)

@marek-safar
Copy link
Contributor

@lambdageek please review

CoffeeFlux
CoffeeFlux reviewed Oct 26, 2020
View reviewed changes
tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs
@@ -154,6 +153,12 @@ private string GenPInvokeDecl(PInvoke pinvoke)
{
var sb = new StringBuilder();
var method = pinvoke.Method;
if (method.Name == "EnumCalendarInfo") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just intended as a temporary hack until that MetadataLoadContext bug is fixed though, so it might be fine for now? Unless you think that patch is landing soon.

tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs Outdated Show resolved Hide resolved
tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs Outdated Show resolved Hide resolved
lambdageek
lambdageek reviewed Oct 26, 2020
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also need to exclude entrypoints.c from the sources in src/mono/mini/CMakeLists.txt. Otherwise gPalGlobalizationNative will still build the array of all the globalization functions.

@runfoapp runfoapp bot mentioned this pull request Oct 26, 2020
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Oct 26, 2020

I think you also need to exclude entrypoints.c from the sources in src/mono/mini/CMakeLists.txt. Otherwise gPalGlobalizationNative will still build the array of all the globalization functions.

It will be built, but nothing will reference it. Will check.

@CoffeeFlux CoffeeFlux mentioned this pull request Oct 26, 2020
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Oct 26, 2020

I think you also need to exclude entrypoints.c from the sources in src/mono/mini/CMakeLists.txt. Otherwise gPalGlobalizationNative will still build the array of all the globalization functions.

It will be built, but nothing will reference it. Will check.

Removing it doesn't seem to matter.

@lewing lewing added the arch-wasm WebAssembly architecture label Oct 26, 2020
vargaz and others added 3 commits October 26, 2020 22:18
@vargaz
Disable qcalls on wasm. Treat them as normal pinvokes.
90e3031 
This is needed because they are stored in a separate table, so they cannot be linked out the same
way as pinvokes/icalls.
@vargaz @CoffeeFlux
Update tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGene…
8ce60eb 
…rator.cs

Co-authored-by: Ryan Lucia <[email protected]>
@vargaz @CoffeeFlux
Update tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGene…
795594f 
…rator.cs

Co-authored-by: Ryan Lucia <[email protected]>
This was referenced Oct 27, 2020
Closed
Closed
@vargaz vargaz merged commit dbe6447 into dotnet:master Oct 28, 2020
@vargaz vargaz deleted the wasm-qcall branch October 28, 2020 01:11
@lewing lewing mentioned this pull request Nov 11, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lambdageek lambdageek lambdageek approved these changes

@marek-safar marek-safar marek-safar left review comments

@lewing lewing lewing left review comments

@CoffeeFlux CoffeeFlux CoffeeFlux approved these changes

@thaystg thaystg Awaiting requested review from thaystg

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-VM-meta-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vargaz @marek-safar @lewing @lambdageek @CoffeeFlux @jkoritzinsky @Dotnet-GitSync-Bot