Move funclets invocation to code managers #114180

janvorli · 2025-04-02T23:08:40Z

This change moves exception handling funclets invocation to the ICodeManager derived classes so that it can be done differently for JITted/crossgened code and interpreted code.

This change moves exception handling funclets invocation to the ICodeManager so that it can be done differently for JITted/crossgened code and interpreted code.

Copilot

Pull Request Overview

This PR moves funclet invocation for exception handling into ICodeManager‑derived classes so that different implementations can be provided for JITted/crossgened versus interpreted code.

Removed direct usage of GetEstablisherFrame and handler function calls in catch, finally, and filter funclet blocks in exceptionhandling.cpp.
Updated EECodeManager in eetwain.cpp and its corresponding interface in eetwain.h to include a CallFunclet virtual method for funclet invocation.

Reviewed Changes

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

File	Description
src/coreclr/vm/exceptionhandling.cpp	Replaced direct funclet calls with CodeManager::CallFunclet invocations.
src/coreclr/vm/eetwain.cpp	Added alternative implementation for CallFunclet and GetEstablisherFrame.
src/coreclr/inc/eetwain.h	Updated the virtual method declarations for CallFunclet.

Copilot · 2025-04-02T23:09:05Z

src/coreclr/vm/exceptionhandling.cpp

-                                   CastHandlerFn(pfnHandler),
-                                   GetFirstNonVolatileRegisterAddress(pvRegDisplay->pCurrentContext),
-                                   pFuncletCallerSP);
+        dwResumePC = exInfo->m_frameIter.m_crawl.GetCodeManager()->CallFunclet(throwable, pHandlerIP, pvRegDisplay, exInfo, false /* isFilterFunclet */);


Ensure that the new CallFunclet invocation in the catch funclet correctly replicates the behavior (especially the resume address) of the previous direct handler call. It may be beneficial to document any behavioral differences introduced by delegating to the CodeManager.

Copilot · 2025-04-02T23:09:05Z

src/coreclr/vm/exceptionhandling.cpp

-    DWORD_PTR dwResumePC = pfnHandler(establisherFrame, NULL);
-#endif
+
+    exInfo->m_frameIter.m_crawl.GetCodeManager()->CallFunclet(NULL, pHandlerIP, pvRegDisplay, exInfo, false /* isFilterFunclet */);


Confirm that discarding the return value from CallFunclet in the finally funclet case is intentional and that no downstream logic depends on it; consider adding a clarifying comment if necessary.

janvorli · 2025-04-02T23:10:21Z

cc: @filipnavara

filipnavara · 2025-04-02T23:32:24Z

src/coreclr/vm/exceptionhandling.cpp

+        dwResumePC = exInfo->m_frameIter.m_crawl.GetCodeManager()->CallFunclet(throwable, pHandlerIP, pvRegDisplay, exInfo, false /* isFilterFunclet */);

+#ifdef USE_FUNCLET_CALL_HELPER
        FixContext(pvRegDisplay->pCurrentContext);


It would make sense to move the FixContext logic inside EECodeManager::CallFunclet. Afterall I don't expect it to be relevant for interpreter (ignoring the fact that we don't support interpreter on x86) and it only calls back into the code manager anyway.

I've actually had it there, but moved it back. It is used for catch funclet only, so I would have to add another flag to the signature to distinguish catch. The context fixing is only relevant to the resuming after catch and that happens here, not in the code manager. I would actually prefer replacing the fixcontext here by its body and ifdef it on x86, since the function does nothing for all other architectures.

janvorli · 2025-04-03T08:06:34Z

There is some Linux problem, I am investigating.

I've missed the fact that the macro is defined in the exceptionhandling.cpp only, so the move to eetwain.cpp disabled this feature.

jkotas

Unix test failures look real

janvorli · 2025-04-04T18:30:09Z

Unix test failures look real

@jkotas Yes, I've been looking into that whole day and finally found the culprit. The C++ compiler optimized the call to the actual funclet from the CallFunclet as a tail call, so the m_csfEHClause wasn't the caller SP. The StackFrameIterator depends on that. I have fixed that by disabling optimizations for the CallFunclet on Windows / disabling tail calling on Unix. I am running the tests now and will update the PR soon.

jkotas · 2025-04-04T20:29:53Z

src/coreclr/vm/eetwain.cpp

+#else
+    HandlerFn* pfnHandler = (HandlerFn*)pHandler;
+
+    pExInfo->m_csfEHClause = CallerStackFrame((UINT_PTR)GetCurrentSP());


Can this line be in the #else of #ifdef USE_FUNCLET_CALL_HELPER?

(It would be nice to update x64 to use the asm helper too at some point, so we should keep all workarounds related to that under the !asm helper ifdef.

Switching x64 to asm helper should improve code size of the EH codegen and remove unnecessary differences between architectures.

src/coreclr/vm/eetwain.cpp

jkotas · 2025-04-04T22:34:44Z

src/coreclr/vm/eetwain.cpp

+
+    pExInfo->m_csfEHClause = CallerStackFrame((UINT_PTR)GetCurrentSP());


Suggested change

pExInfo->m_csfEHClause = CallerStackFrame((UINT_PTR)GetCurrentSP());

src/coreclr/vm/eetwain.cpp

Move funclets invocation to code managers

f5acd7a

This change moves exception handling funclets invocation to the ICodeManager so that it can be done differently for JITted/crossgened code and interpreted code.

janvorli added the area-ExceptionHandling-coreclr label Apr 2, 2025

janvorli added this to the 10.0.0 milestone Apr 2, 2025

janvorli requested review from kg, cshung, BrzVlad and jkotas April 2, 2025 23:08

janvorli self-assigned this Apr 2, 2025

Copilot AI review requested due to automatic review settings April 2, 2025 23:08

Copilot AI reviewed Apr 2, 2025

View reviewed changes

filipnavara reviewed Apr 2, 2025

View reviewed changes

Fix m_csfEHClause - it needs to be SP of the direct funclet caller

dd389a6

build-analysis bot mentioned this pull request Apr 3, 2025

[linux-x64] [mono-aot] Test Runtime_101731.TestConvertToInt64NativeSingle(3.4028235E+38) returns exit code 22 #112557

Open

Fix build for platforms with USE_FUNCLET_CALL_HELPER

9d7af16

I've missed the fact that the macro is defined in the exceptionhandling.cpp only, so the move to eetwain.cpp disabled this feature.

This was referenced Apr 4, 2025

System.Net.Requests test timeout #113883

Closed

System.Net.Security.Unit.Tests timeout #114176

Closed

Fix WASM build

81dd7cb

This was referenced Apr 4, 2025

System.Net.Quic tests timeout #107761

Closed

System.Net.Security.Tests timeout #114152

Closed

System.Net.WebSockets.Client.Tests timeout #114153

Closed

jkotas approved these changes Apr 4, 2025

View reviewed changes

jkotas requested changes Apr 4, 2025

View reviewed changes

Fix random crashes due to tail call optimization

e797e63

jkotas reviewed Apr 4, 2025

View reviewed changes

src/coreclr/vm/eetwain.cpp Show resolved Hide resolved

jkotas reviewed Apr 4, 2025

View reviewed changes

src/coreclr/vm/eetwain.cpp Outdated Show resolved Hide resolved

Fix build with GCC and reflect feedback

8378130

am11 reviewed Apr 4, 2025

View reviewed changes

src/coreclr/vm/eetwain.cpp Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 4, 2025

[QUIC & HTTP/3] Handshake Timeout on tests #104426

Closed

jkotas reviewed Apr 4, 2025

View reviewed changes

src/coreclr/vm/eetwain.cpp Outdated Show resolved Hide resolved

jkotas and others added 2 commits April 4, 2025 15:06

Update src/coreclr/vm/eetwain.cpp

79efd27

Address feedback

30676ee

jkotas reviewed Apr 4, 2025

View reviewed changes

src/coreclr/vm/eetwain.cpp Show resolved Hide resolved

Another feedback

9203881

jkotas approved these changes Apr 4, 2025

View reviewed changes

Merge branch 'main' into move-funclet-calls-to-code-manager

f5a4e79

janvorli merged commit e72f368 into dotnet:main Apr 7, 2025
98 checks passed

janvorli deleted the move-funclet-calls-to-code-manager branch April 7, 2025 13:24

github-actions bot locked and limited conversation to collaborators May 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move funclets invocation to code managers #114180

Move funclets invocation to code managers #114180

Uh oh!

janvorli commented Apr 2, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2025

Uh oh!

Copilot AI Apr 2, 2025

Uh oh!

janvorli commented Apr 2, 2025

Uh oh!

filipnavara Apr 2, 2025

Uh oh!

janvorli Apr 2, 2025

Uh oh!

janvorli commented Apr 3, 2025

Uh oh!

jkotas left a comment

Uh oh!

janvorli commented Apr 4, 2025

Uh oh!

jkotas Apr 4, 2025

Uh oh!

jkotas Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		pExInfo->m_csfEHClause = CallerStackFrame((UINT_PTR)GetCurrentSP());

Move funclets invocation to code managers #114180

Move funclets invocation to code managers #114180

Uh oh!

Conversation

janvorli commented Apr 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 2, 2025

Uh oh!

filipnavara Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

janvorli Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 3, 2025

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 4, 2025

Uh oh!

jkotas Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!