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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90e3031
Disable qcalls on wasm. Treat them as normal pinvokes.
vargaz Oct 24, 2020
8ce60eb
Update tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGene…
vargaz Oct 26, 2020
795594f
Update tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGene…
vargaz Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/mono/cmake/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,9 @@
/* Icall tables disabled */
#cmakedefine DISABLE_ICALL_TABLES 1

/* QCalls disabled */
#cmakedefine DISABLE_QCALLS 1

/* Have __thread keyword */
#cmakedefine MONO_KEYWORD_THREAD 1

Expand Down
1 change: 1 addition & 0 deletions src/mono/cmake/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ option (DISABLE_EVENTPIPE "Disable EventPipe support")
option (DISABLE_EXECUTABLES "Disable the build of the runtime executables")
option (DISABLE_CRASH_REPORTING "Disable crash reporting subsystem")
option (DISABLE_ICALL_TABLES "Enable separate icall table library")
option (DISABLE_QCALLS "Disable support for QCalls")
option (ENABLE_ICALL_EXPORT "Export icall functions")
option (ENABLE_ICALL_SYMBOL_MAP "Generate tables which map icall functions to their C symbols")
option (ENABLE_PERFTRACING "Enables support for eventpipe library")
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@
</ItemGroup>
<!-- WASM specific options -->
<ItemGroup Condition="'$(TargetsBrowser)' == 'true'">
<_MonoCMakeArgs Include="-DENABLE_MINIMAL=ssa,com,jit,reflection_emit_save,portability,assembly_remapping,attach,verifier,full_messages,appdomains,shadowcopy,security,sgen_marksweep_conc,sgen_split_nursery,sgen_gc_bridge,sgen_toggleref,logging,remoting,shared_perfcounters,sgen_debug_helpers,sgen_binary_protocol,soft_debug,interpreter,assert_messages,cleanup,mdb,gac,threads,eventpipe,aot,interpreter"/>
<_MonoCMakeArgs Include="-DENABLE_MINIMAL=ssa,com,jit,reflection_emit_save,portability,assembly_remapping,attach,verifier,full_messages,appdomains,shadowcopy,security,sgen_marksweep_conc,sgen_split_nursery,sgen_gc_bridge,sgen_toggleref,logging,remoting,shared_perfcounters,sgen_debug_helpers,sgen_binary_protocol,soft_debug,interpreter,assert_messages,cleanup,mdb,gac,threads,eventpipe,aot,interpreter,qcalls"/>
<_MonoCMakeArgs Include="-DENABLE_INTERP_LIB=1"/>
<_MonoCMakeArgs Include="-DDISABLE_ICALL_TABLES=1"/>
<_MonoCMakeArgs Include="-DDISABLE_CRASH_REPORTING=1"/>
Expand Down
2 changes: 2 additions & 0 deletions src/mono/mono/metadata/native-library-qcall.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ const void* gPalGlobalizationNative[] = { (void*)func_flag_end_of_array };

static const MonoQCallDef c_qcalls[] =
{
#ifndef DISABLE_QCALLS
#define FCClassElement(name,namespace,funcs) {name, namespace, funcs},
#include "mono/metadata/qcall-def.h"
#undef FCClassElement
#endif
};

const int c_nECClasses = sizeof (c_qcalls) / sizeof (c_qcalls[0]);
Expand Down
3 changes: 3 additions & 0 deletions src/mono/mono/metadata/native-library.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,8 @@ lookup_pinvoke_call_impl (MonoMethod *method, MonoLookupPInvokeStatus *status_ou
new_import = g_strdup (orig_import);
#endif

/* If qcalls are disabled, we fall back to the normal pinvoke code for them */
#ifndef DISABLE_QCALLS
if (strcmp (new_scope, "QCall") == 0) {
piinfo->addr = mono_lookup_pinvoke_qcall_internal (method, status_out);
if (!piinfo->addr) {
Expand All @@ -1332,6 +1334,7 @@ lookup_pinvoke_call_impl (MonoMethod *method, MonoLookupPInvokeStatus *status_ou
}
return piinfo->addr;
}
#endif

#ifdef ENABLE_NETCORE
#ifndef HOST_WIN32
Expand Down
1 change: 1 addition & 0 deletions src/mono/wasm/wasm.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

<WasmPInvokeAssembly Include="@(LibrariesRuntimeFiles)" Condition="'%(Extension)' == '.dll' and '%(IsNative)' != 'true'" />
</ItemGroup>

Expand Down
11 changes: 8 additions & 3 deletions tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ public class PInvokeTableGenerator : Task

public override bool Execute()
{
Log.LogMessage(MessageImportance.Normal, $"Generating pinvoke table to '{OutputPath}'.");
GenPInvokeTable(Modules!.Select(item => item.ItemSpec).ToArray(), Assemblies!.Select(item => item.ItemSpec).ToArray());
return true;
}

private void GenPInvokeTable(string[] pinvokeModules, string[] assemblies)
public void GenPInvokeTable(string[] pinvokeModules, string[] assemblies)
{
var modules = new Dictionary<string, string>();
foreach (var module in pinvokeModules)
Expand All @@ -46,8 +47,6 @@ private void GenPInvokeTable(string[] pinvokeModules, string[] assemblies)
CollectPInvokes(pinvokes, callbacks, type);
}

Log.LogMessage(MessageImportance.Normal, $"Generating pinvoke table to '{OutputPath}'.");

using (var w = File.CreateText(OutputPath!))
{
EmitPInvokeTable(w, modules, pinvokes);
Expand Down Expand Up @@ -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)

// FIXME: System.Reflection.MetadataLoadContext can't decode function pointer types
// https://github.com/dotnet/runtime/issues/43791
sb.Append($"int {pinvoke.EntryPoint} (int, int, int, int, int);");
return sb.ToString();
}
sb.Append(MapType(method.ReturnType));
sb.Append($" {pinvoke.EntryPoint} (");
int pindex = 0;
Expand Down