Skip to content

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Aug 10, 2025

A follow-up for #109806

Most of the implementation is in the shared corelib code and works for Mono, but finalization scenario needs special handling as that part is not shared.

@VSadov VSadov added the runtime-mono specific to the Mono runtime label Aug 10, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 10, 2025
@dotnet-policy-service dotnet-policy-service bot added linkable-framework Issues associated with delivering a linker friendly framework labels Aug 10, 2025
@VSadov VSadov added area-VM-meta-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 12, 2025
@VSadov VSadov marked this pull request as ready for review August 13, 2025 01:54
@Copilot Copilot AI review requested due to automatic review settings August 13, 2025 01:54
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the UnhandledException hook for finalizer scenarios in Mono runtime, building on previous work in shared corelib code. The implementation ensures that unhandled exceptions in finalizers are properly handled through the global exception handler mechanism.

Key changes:

  • Introduces a new GuardedFinalize method that wraps finalizer calls with exception handling
  • Updates Mono's finalizer execution path to use the guarded version instead of direct finalizer calls
  • Enables previously excluded UnhandledException tests for Mono (with some exceptions)

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/tests/issues.targets Enables UnhandledException tests for Mono, excluding only specific problematic test cases
src/mono/mono/mini/aot-compiler.c Updates AOT compilation to use GuardedFinalize instead of Finalize for runtime-invoke
src/mono/mono/metadata/gc.c Modifies finalizer execution to call GuardedFinalize and updates method resolution
src/mono/System.Private.CoreLib/src/System/Object.Mono.cs Implements GuardedFinalize method with exception handling using UnsafeAccessor
src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml Preserves GuardedFinalize method from IL linking removal

@VSadov
Copy link
Member Author

VSadov commented Aug 13, 2025

I think this is ready for a review.

There are some failures on ios/tvos, but I cannot tell if these are caused by the change.
In theory the approach should work on all platforms. - it seems to work even for WASM

@VSadov
Copy link
Member Author

VSadov commented Aug 14, 2025

This keeps failing on ios/tvos. I am out of ideas why. @lambdageek - could you take a look?

@lambdageek
Copy link
Member

lambdageek commented Aug 14, 2025

@VSadov I can't really see anything in the CI logs, but probably something needs to tell Mono's AOT compiler to compile GuardedFinalize

@kotlarmilos
Copy link
Member

Let me run it locally to try to get better diagnostics/ stack trace.

@kotlarmilos
Copy link
Member

I am getting the following error on iOS arm64 device:

((null) error) * Assertion at /Users/miloskotlar/dotnet/runtime-unsafe-accessor/src/mono/mono/metadata/gc.c:304, condition `is_ok (error)' not met, function:mono_gc_run_finalize, Attempting to JIT compile method '(wrapper runtime-invoke) object object:runtime_invoke_direct_void__this__ (object,intptr,intptr,intptr)' while running in aot-only mode. See https://learn.microsoft.com/xamarin/ios/internals/limitations for more information.

@VSadov
Copy link
Member Author

VSadov commented Aug 15, 2025

I am getting the following error on iOS arm64 device:

((null) error) * Assertion at /Users/miloskotlar/dotnet/runtime-unsafe-accessor/src/mono/mono/metadata/gc.c:304, condition `is_ok (error)' not met, function:mono_gc_run_finalize, Attempting to JIT compile method '(wrapper runtime-invoke) object object:runtime_invoke_direct_void__this__ (object,intptr,intptr,intptr)' while running in aot-only mode. See https://learn.microsoft.com/xamarin/ios/internals/limitations for more information.

I think I am precompiling the wrapper. We had the same pattern before, just the wrapper was for Finalize and now it is for GuardedFinalize.
I wonder what is the difference?

The wrapper for Finalize was a virtual wrapper, since Finalize is a virtual method. The GuardedFinalize is not virtual, since it does not need to be.
I can make GuardedFinalize virtual and see what happens.

@VSadov
Copy link
Member Author

VSadov commented Aug 15, 2025

Making GuardedFinalize virtual actually helps with ios/tvos. So it is not an issue with UnsafeAccessor. It is an issue with precompiling direct nonvirtual invoke wrapper.

Virtual GuardedFinalize could be ok, but less than ideal.
I will now try a nonvirtual again, but with non-direct wrapper.

@VSadov
Copy link
Member Author

VSadov commented Aug 18, 2025

LLVMFULLAOT fails in Regression_2 JIT testcase. I do not think it is related.

The rest is passing now, including the newly enabled tests for UnhandledExceptionHandler - that is on all tested configs: JIT, interpreted, WASM or AOT/ios variants.

@VSadov
Copy link
Member Author

VSadov commented Aug 19, 2025

/ba-g LLVMFULLAOT failure in Regression_2 is unrelated. Happens without this change as well (see: NOOP change in #118642)

@VSadov
Copy link
Member Author

VSadov commented Aug 19, 2025

Thanks for reviewing!

@VSadov VSadov merged commit de0650b into dotnet:main Aug 19, 2025
130 of 132 checks passed
@VSadov
Copy link
Member Author

VSadov commented Aug 19, 2025

/backport to release/10.0-rc1

Copy link
Contributor

Started backporting to release/10.0-rc1: https://github.com/dotnet/runtime/actions/runs/17064173533

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-mono linkable-framework Issues associated with delivering a linker friendly framework runtime-mono specific to the Mono runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants