Modifying wasm interop for exception handling#132286
Conversation
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR tightens CoreCLR-on-Wasm P/Invoke exception-boundary behavior by introducing a dedicated JIT helper and new wasm EH emission so that foreign/unmanaged exceptions don’t unwind into managed code, while emitting a clear “unmanaged exception escaped” message.
Changes:
- Add a new JIT/ReadyToRun helper (
CORINFO_HELP_REPORT_UNMANAGED_EXCEPTION_FROM_PINVOKE/ReportUnmanagedExceptionFromPInvoke) and wire it through the JIT interface + AOT/R2R tooling. - Extend wasm emitter/JIT support with
catch_all_refand emittry_tablewrapping around unmanaged calls to distinguish managed vs foreign exceptions. - Add a wasm build test validating that an unmanaged exception escaping a P/Invoke does not reach managed
catchand terminates with the expected message.
Show a summary per file
| File | Description |
|---|---|
| src/mono/wasm/Wasm.Build.Tests/PInvokeTableGeneratorTests.cs | Adds a regression test asserting unmanaged exceptions don’t unwind through managed code on CoreCLR Wasm. |
| src/coreclr/vm/wasm/helpers.cpp | Implements JIT_ReportUnmanagedExceptionFromPInvoke to print the boundary violation message. |
| src/coreclr/vm/jithelpers.cpp | Adds a wasm-only forward declaration for the new helper. |
| src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs | Adds the new JIT helper ID to the managed JIT interface enum. |
| src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs | Adds the new ReadyToRun helper constant for tooling. |
| src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs | Maps the new CorInfo helper to the ReadyToRun helper ID for NativeAOT RyuJit. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunSignature.cs | Adds decoding/display of the new helper in R2R signature pretty-printing. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Maps the new CorInfo helper to the ReadyToRun helper ID for R2R compilation. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/JitHelper.cs | Resolves the new helper ID to the exported symbol name for AOT/R2R. |
| src/coreclr/jit/utils.cpp | Marks the new helper as non-throwing from the JIT’s perspective. |
| src/coreclr/jit/instrswasm.h | Introduces catch_all_ref instruction metadata for wasm codegen. |
| src/coreclr/jit/emitwasm.cpp | Implements sizing/encoding/display support for the new catch-all declaration format. |
| src/coreclr/jit/emitfmtswasm.h | Adds the new IF_CATCH_ALL_DECL format definition. |
| src/coreclr/jit/codegenwasm.cpp | Wraps unmanaged calls in try_table with catch_ref + catch_all_ref and invokes the new helper on foreign exceptions. |
| src/coreclr/inc/readytorunhelpers.h | Adds the helper mapping for ReadyToRun helper tables. |
| src/coreclr/inc/readytorun.h | Adds the new helper to the native ReadyToRunHelper enum. |
| src/coreclr/inc/jithelpers.h | Registers the new helper in the JIT helper table (wasm implementation / other targets NULL). |
| src/coreclr/inc/jiteeversionguid.h | Bumps the JIT/EE versioning GUID to reflect the interface change. |
| src/coreclr/inc/corinfo.h | Adds the new helper ID to the native CorInfoHelpFunc enum. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
| EXTERN_C void JIT_ReportUnmanagedExceptionFromPInvoke() | ||
| { | ||
| fprintf(stderr, "Unhandled exception: an unmanaged exception was thrown out of a managed-to-native transition\n"); | ||
| } |
| Assert.Contains( | ||
| result.ConsoleOutput, | ||
| line => line.Contains("Unhandled exception: an unmanaged exception was thrown out of a managed-to-native transition")); | ||
| Assert.DoesNotContain(result.TestOutput, line => line.Contains("managed catch")); |
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/mono/wasm/Wasm.Build.Tests/PInvokeTableGeneratorTests.cs:374
- Prefer xUnit conditional attributes over runtime early-returns for skipping tests. This repo already uses ConditionalTheory with BuildTestBase.IsCoreClrRuntime (e.g., InvariantGlobalizationTests.cs:46), so this should follow the same pattern instead of
if (!IsCoreClrRuntime) return;.
[Theory]
[InlineData(Configuration.Debug)]
public async Task UnmanagedExceptionDoesNotUnwindThroughManagedCode(Configuration config)
{
if (!IsCoreClrRuntime)
src/coreclr/jit/utils.cpp:1747
CORINFO_HELP_REPORT_UNMANAGED_EXCEPTION_FROM_PINVOKEmaps to a DECLSPEC_NORETURN helper (JIT_ReportUnmanagedExceptionFromPInvoke). HelperCallProperties should mark this as non-returning (alwaysThrow = true), similar to CORINFO_HELP_FAIL_FAST, so the JIT doesn't treat the call as a normal fallthrough.
case CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT:
case CORINFO_HELP_JIT_PINVOKE_BEGIN:
case CORINFO_HELP_JIT_PINVOKE_END:
case CORINFO_HELP_REPORT_UNMANAGED_EXCEPTION_FROM_PINVOKE:
exceptions = ExceptionSetFlags::None;
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
| regNumber thisReg = REG_NA; | ||
|
|
||
| WasmValueType callResultType = WasmValueType::Invalid; | ||
| if (call->IsUnmanaged()) |
There was a problem hiding this comment.
Does this work for QCalls?
We do make exception interop work for QCalls since we own both sides. QCalls can throw exception in C++ that automatically shows up as a regular managed exception in C#.
| regNumber thisReg = REG_NA; | ||
|
|
||
| WasmValueType callResultType = WasmValueType::Invalid; | ||
| if (call->IsUnmanaged()) |
There was a problem hiding this comment.
What's the perf overhead of emitting explicit try/catch around every PInvoke? Instead of this, can we detect the unexcepted exception in the try/catch handler upstack?
|
I think this may break R2R Wasm EH. We rely on Wasm exceptions to unwind the stack back to the catching frame. If this catches all exceptions it will need to rethrow the exception we use for unwinding. |
|
What is the motivation for dealing with escaping foreign exceptions for WASM specifically? They're UB on other Unix targets. Presumably the main channel for exceptions on WASM is JS, not "C++ compiled to WASM" PInvokes. And we have a separate interop layer for them... |
|
Yeah, copilot got a little too eager here -- this was supposed ot be an experiment. |
Pull request created by AI Agent