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

[wasm] pinvoke improvements part 2 #98250

Merged
merged 21 commits into from
Feb 21, 2024
Merged

[wasm] pinvoke improvements part 2 #98250

merged 21 commits into from
Feb 21, 2024

Conversation

kg
Copy link
Member

@kg kg commented Feb 9, 2024

Expands WBT test coverage and makes the following improvements:

  • InlineArray is supported in wasm pinvokes, and in general works correctly in mono marshaling now (it was broken on all targets)
  • fixed arrays are supported in wasm pinvokes
  • struct arguments and return values are supported in more wasm pinvoke scenarios (not all, though)
  • wasm struct scalarization is more intelligent about noticing padding and size mismatches in structs (without this, some inlinearrays would be scalarized incorrectly)
  • Prevent infinite recursion in build task's TypeToChar

@kg kg added the arch-wasm WebAssembly architecture label Feb 9, 2024
@ghost
Copy link

ghost commented Feb 9, 2024

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

Issue Details

Expands WBT test coverage and makes the following improvements:

  • InlineArray is supported in wasm pinvokes, and in general works correctly in mono marshaling now (it was broken on all targets)
  • fixed arrays are supported in wasm pinvokes
  • struct arguments and return values are supported in more wasm pinvoke scenarios (not all, though)
  • wasm struct scalarization is more intelligent about noticing padding and size mismatches in structs (without this, some inlinearrays would be scalarized incorrectly)
Author: kg
Assignees: -
Labels:

arch-wasm

Milestone: -

@kg
Copy link
Member Author

kg commented Feb 15, 2024

cc @vargaz there is some debugging code in here now, the problem i'm digging into is that System.Runtime.Tests fails in AOT mode.
Part of the the problem appears to be that Assert.Equals drops into the interpreter, and then at some point we call Task.GetAwaiter from the interpreter, and the jitcall to do that appears to use the wrong signature - it expects (ii) -> i, and it seems like the target is (iiiii) or (iii), so there's probably a disagreement somewhere on whether TaskAwaiter gets scalarized. TaskAwaiter has one field of type Task so it probably should be scalarized, but we might not be allowing that for reftypes in all the scalar vtype code we wrote. I'm not sure whether scalarizing a GC reference is safe since it wouldn't be visible to the collector anymore.

Failure log trimmed:

  info: jit_call_cb 15835 (2 args + ftndesc)
  info: do_jit_call entering AssertEqualityComparer`1::Equals
  info: jit_call_cb 18ac0 (7 args + ftndesc)
  info: do_jit_call entering Task`1::GetAwaiter
  info: jit_call_cb 15bad (2 args + ftndesc)
  info: /home/kate/Projects/dotnet-runtime-wasm/artifacts/bin/System.Runtime.Tests/Release/net9.0-browser/browser-wasm/AppBundle/_framework/dotnet.native.js:196: RuntimeError: null function or function signature mismatch
  info:       throw toThrow;
  info:       ^
  info: RuntimeError: null function or function signature mismatch
  info:     at aot_instances_aot_wrapper_gsharedvt_out_sig_cl1d_Mono_dValueTuple_601_3cint_3e__this_ (wasm://wasm/0e443d3e:wasm-function[90317]:0x16fbf96)
  info:     at jit_call_cb (wasm://wasm/0e443d3e:wasm-function[106223]:0x1a5651f)
  info:     at mono_llvm_cpp_catch_exception (wasm://wasm/0e443d3e:wasm-function[111704]:0x1b7143f)
  info:     at mono_llvm_catch_exception (wasm://wasm/0e443d3e:wasm-function[111771]:0x1b7540f)
  info:     at do_jit_call (wasm://wasm/0e443d3e:wasm-function[106140]:0x1a530d8)
  info:     at mono_interp_exec_method (wasm://wasm/0e443d3e:wasm-function[106135]:0x1a47369)
  info:     at interp_entry (wasm://wasm/0e443d3e:wasm-function[106214]:0x1a55f42)
  info:     at interp_entry_instance_0 (wasm://wasm/0e443d3e:wasm-function[106239]:0x1a57220)
  info:     at aot_instances_aot_wrapper_gsharedvt_in_sig_void_this_ (wasm://wasm/0e443d3e:wasm-function[104999]:0x1a037ab)
  info:     at corlib_System_Runtime_CompilerServices_AsyncMethodBuilderCore_Start_TStateMachine_GSHAREDVT_TStateMachine_GSHAREDVT_ (wasm://wasm/0e443d3e:wasm-function[35187]:0x6cb566)
  info:     at aot_instances_Xunit_Sdk_ExceptionAggregator_RunAsync_System_Decimal_System_Func_1_System_Threading_Tasks_Task_1_System_Decimal (wasm://wasm/0e443d3e:wasm-function[92577]:0x1779af3)
  info:     at xunit_execution_dotnet_Xunit_Sdk_TestInvoker_1_TTestCase_REF_RunAsync (wasm://wasm/0e443d3e:wasm-function[75067]:0x13bbfc1)
  info:     at xunit_execution_dotnet_Xunit_Sdk_XunitTestRunner_InvokeTestMethodAsync_Xunit_Sdk_ExceptionAggregator (wasm://wasm/0e443d3e:wasm-function[75293]:0x13ce87b)
  info:     at xunit_execution_dotnet_Xunit_Sdk_XunitTestRunner__InvokeTestAsyncd__4_MoveNext (wasm://wasm/0e443d3e:wasm-function[75294]:0x13cec86)

if (underlyingType != t)
c = TypeToChar(underlyingType, log, out _, ++depth);
else {
log.Warning("WASM0064", $"Unsupported parameter type '{t.Name}'");
Copy link
Member

Choose a reason for hiding this comment

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

When can this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know of any realistic scenario where unbounded recursion can occur in TypeToChar, but it broke the wasm perf measurements so I am doing everything I can to fix it. I can't repro it locally.

@vargaz
Copy link
Contributor

vargaz commented Feb 17, 2024

The runtime changes look ok to me.

@kg kg merged commit b8d5346 into dotnet:main Feb 21, 2024
111 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-meta-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants