Add support for building runtime with FEATURE_EH_FUNCLETS on win-x86

[filipnavara]

This doesn't change the runtime configuration but it enables the code paths necessary to support it in future if we decide to switch.

Peeled off from #113576, enabling the funclet build would require the following changes: fde468b

Contributes to #113985

[filipnavara]

The flags between funclet and non-funclet are overloaded. We are building JIT without FEATURE_EH_FUNCLETS but it can produce both funclet and non-funclet code. It's a preexisting issue that JitDumps on NativeAOT/win-x86 would trigger the asserts.

[jkotas]

It looks great to me! Thanks a lot for working on this.

[jkotas]

@janvorli Could you please take look as well?

[janvorli]

I believe this can go away, with the recent removal of the ProcessCLRException personality routine from asm helpers, we should never reach this place for non-managed code frames.
But let me double check that we don't get here for some dynamically generated helpers that are not managed code. I don't think we would, but I want to be sure. So let's keep this as is and I'll remove it in a separate PR.

[janvorli]

I am not sure I understand the reason for this change and also why would the CallDescrWorkerInternal have ProcessCLRException personality routine.

[filipnavara]

win-x86 has no unwinding data and no personality routines associated with code blocks, so we use a scheme similar to the current one. A SEH record is pushed on the stack for each entry into a managed call stack. That's either CallDescrWorkerInternal or reverse P/Invoke. For reverse P/Invoke the SEH record is actually inside the managed code frame, so let's ignore that for a moment, since that's a situation closer to other platforms.

For CallDescrWorkerInternal we need to push the SEH frame for the managed code to the stack before the managed code starts running. We don't have a managed code stub that would do it and naturally associate the personality routine with some managed code. We push the frame with ProcessCLRException early and then back-patch it to point to CallDescrWorkerUnwindFrameChainHandler in two cases:

1. Prestub which compiles the managed code and can throw C++ exception. These exceptions need to be passed up the stack since they get wrapped in type/class loader exceptions before they are passed into managed code.
2. When managed exception is propagated back through native code, we pretend that the personality routine for CallDescrWorkerInternal was CallDescrWorkerUnwindFrameChainHandler all along so it properly cleans up the tracked exceptions. We cannot leave ProcessCLRException there because it would unwind the thread's frames and start processing the next managed code chunk and break any C++ catch of managed exception.

[jkotas]

Prestub which compiles the managed code and can throw C++ exception.

Do all other places that use INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX macro have the same problem? Should we use INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX macro to patch this up?

[filipnavara]

VSD_ResolveWorker only throws NullReferenceException so we didn't hit the problem there. I am not sure if there's any real scenario where we would need to account for that. If yes, then it's not covered by Pri0 CoreCLR tests. It would not be difficult to handle it in the assembly helper the same way as ThePreStub.

I tried to use INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX at some point but it introduces more issues than it solves. MSVC doesn't allow mixing SEH and C++ exception handling in the same method (on non-x86 it would not be representable without complex code transformation due to conflict between personality routines), and installing SEH chain record manually from C++ code seemed dangerous at best. Debug contracts also install exception handlers into the chain and it's difficult to reason about the order and correctness.

[janvorli]

I am still not sure why we need to have the ProcessCLRException there initially instead of the having there the CallDescrWorkerUnwindFrameChainHandler from the very beginning. Can you please help me understand the case where the ProcessCLRException would be needed?

[jkotas]

VSD_ResolveWorker only throws NullReferenceException

VSD_ResolveWorker can throw all sorts of exceptions.

This looks very similar to the #112666 that introduced the INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX macro. The PR has some tests to trigger some of the interesting situations.

installing SEH chain record manually from C++ code seemed dangerous at best.

You may be able to edit the handler in the FS:0 chain in INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX macro to cover all cases, instead of editing it in the asm code.

[filipnavara]

VSD_ResolveWorker can throw all sorts of exceptions.

I missed the second usage of INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX inside VSD_ResolveWorker. Thanks for pointing it out.

The PR has some tests to trigger some of the interesting situations.

The tests did pass on multiple runs, both on CI and Release/Checked/Debug local builds. I'll put it on my backlog to check them again and make a note in #113985 to revisit it and check the behavior.

You may be able to edit the handler in the FS:0 chain in INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX macro to cover all cases, instead of editing it in the asm code.

It's possible but it's not going to be efficient. The contracts at the top of VSD_ResolveWorker install 3 (IIRC) SEH frames in Debug builds and I'd have to implement logic to skip over them.

[jkotas]

The contracts at the top of VSD_ResolveWorker install 3 (IIRC) SEH frames in Debug builds and I'd have to implement logic to skip over them.

Assuming that you would just walk a bit of the FS:0 chain to find the right one to patch, it is nothing compared to what VSD_ResolveWorker and other similar methods do.

[filipnavara]

I agree it's tempting in terms of code unification. Is it fine to revisit it later and proceed with this PR as is, or do you prefer to fix it now and re-run the tests?

[jkotas]

It is fine to do in a follow up.

[janvorli]

LGTM, thank you!