Port NativeAOT exception handling to CoreCLR by janvorli · Pull Request #88034 · dotnet/runtime

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli · 2023-06-26T09:49:29Z

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when DOTNET_EnableNewExceptionHandling env variable is set. By default, the old way is used.
This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture.

Notes for the PR:

I have left the ExceptionHandling.cs and StackFrameIterator.cs in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here.
Naming of the native helpers like RhpCallCatchFunclet was left the same as in the NativeAOT for now.
There are still some little things I'd like to eventually clean up, like ExInfo encapsulation and possibly moving REGDISPLAY and CONTEXT it uses into the ExInfo itself or moving debug members of StackFrameIterator and REGDISPLAY to the end of those structures so that the AsmOffsets.cs can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated.
There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR.
- ProfilerEnter and ProfilerLeave for the case of UnmanagedCallersOnly method were being called in preemptive mode.
- NativeAOT code for rethrowing exception was incorrectly calling DispatchEx with last argument set to activeExInfo._idxCurClause to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too.
Changes in the stackwalk.cpp add support for
- Usage of ExInfo instead of ExceptionTracker
- Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable.
- Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case.
I needed to add GCFrame::Remove method that can remove the GCFrame from any location in the chain. There is a managed runtime method that calls GCReporting::Unregister that was popping it with my changes out of order due to the exception handling code being managed.

danmoseley · 2023-06-28T00:36:02Z

How do the perf improvements compare between OS?

In a typical workload, would we expect this to only be noticeable in "exception storms" (e.g. due loss of connectivity)?

jkotas · 2023-06-28T02:53:41Z

How do the perf improvements compare between OS?

#77568 (comment)

janvorli · 2023-06-28T06:33:46Z

I have found (by running a separate testing PR (#88113) just for the NativeAOT fix for rethrowing, that that fix is not correct in all cases. So clearly the last argument of the DispatchEx needs to be sometimes activeExInfo._idxCurClause and sometimes MaxTryRegionIdx. After debugging the issue that lead me to change this, I was convinced that it should always be MaxTryRegionIdx, which means to start at the first clause. So I'll need to debug the case failing in the PR to see why it is not the case here.
For the current PR, I am going to revert this change. The failure I have seen (exception going unhandled) was not happening in any of the tests we have, I was just lucky to create a test myself that was failing without the change for both NativeAOT and the new EH.

The test I have created is as simple as this. The rethrown exception is unhandled. When I comment out the throw; and uncomment the `throw new ArgumentException("aE");', it works.

classProgram{staticvoidMain(string[]args){try{thrownewException("boo");}catch(Exceptionex2){Console.WriteLine($"1: {ex2}");try{throw;//throw new ArgumentException("aE");}catch(Exceptionex3){Console.WriteLine($"2: {ex3}");}}}}

When running result of dotnet publish -c Release -p:PublishAot=true
1: System.Exception: boo
at Program.Main(String[]) + 0x4c
Unhandled Exception: System.Exception: boo
at Program.Main(String[]) + 0x151
at failingnativeaot!<BaseAddress>+0x16bc2b

janvorli · 2023-06-28T10:27:38Z

Fortunately, I have found what was causing the NativeAOT test suite to fail with my rethrow fix. It was just a problem of stack trace, the actual fix is correct.

janvorli · 2023-06-28T12:30:48Z

I have created a couple of PRs to separate bug fixes and cleanups unrelated to this PR.

davidwrighton · 2023-07-26T22:27:02Z

What is this TODO about?

Should it be a TODO-NewEH

Right, I still plan to add a number of TODO-NewEHs to this PR. As for this TODO, the comment was related to the fact that the ExceptionTracker::MakeCallbacksRelatedToHandler, which is the old EH variant of this function, calls m_EHClauseInfo.ResetInfo() and IIRC, profiler stuff somehow relied on more stuff from the m_EHClauseInfo info than the stuff I've extracted from it and use in the new EH.

This change ports NativeAOT exception handling to CoreCLR, resulting in 3.5..4 times speedup in exception handling. Due to time constraints and various complexities, thread abort and debugger support is not completed yet, so this change is enabled only when `DOTNET_EnableNewExceptionHandling` env variable is set. By default, the old way is used. This change supports all OSes and targets we support except of x86 Windows. That may be doable too in the future, but the difference in exception handling on x86 Windows adds quite a lot of complexity into the picture. Notes for the PR: * I have left the `ExceptionHandling.cs` and `StackFrameIterator.cs` in the nativeaot folder to simplify the review. I can move it to some common location after the change is reviewed. Also it was not clear to me where that should be, so advise would be welcome here. * Naming of the native helpers like `RhpCallCatchFunclet` was left the same as in the NativeAOT for now. * There are still some little things I'd like to eventually clean up, like `ExInfo` encapsulation and possibly moving `REGDISPLAY` and `CONTEXT` it uses into the `ExInfo` itself or moving debug members of `StackFrameIterator` and `REGDISPLAY` to the end of those structures so that the `AsmOffsets.cs` can be simplified. It also may be possible to unify the exception handling callback that's used for ObjectiveC to use the managed version. I've tried and there were some ugly complications, so I've left it separated. * There are two bug fixes for bugs unrelated to this PR and a removal of unused parameter in existing code that could be made as separate PRs before this PR. * `ProfilerEnter` and `ProfilerLeave` for the case of `UnmanagedCallersOnly` method were being called in preemptive mode. * NativeAOT code for rethrowing exception was incorrectly calling `DispatchEx` with last argument set to `activeExInfo._idxCurClause` to start at the last clause processed when the rethrown exception was originally thrown instead of starting from the first one again. I have a accidentally came with a simple test that discovered this bug and causes failures in the original NativeAOT too. * Changes in the stackwalk.cpp add support for * Usage of `ExInfo` instead of `ExceptionTracker` * Handling of case when GC runs while finally funclet is on the stack and then again when the code is back in the new exception handling code in managed code before other finally or catch funclet is called. The NativeAOT solves that by disabling GC for the 2nd pass of EH, for this change it would not be reasonable. * Handling the GC reporting when funclet is found while walking the stack. It needs to scan frames of the managed code that handles the exception too, since it contains live references. The old EH way doesn't have this case. * I needed to add `GCFrame::Remove` method that can remove the `GCFrame` from any location in the chain. There is a managed runtime method that calls `GCReporting::Unregister` that was popping it with my changes out of order due to the exception handling code being managed. Fix context initialization after rebase

The `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` in the `EE_TO_JIT_TRANSITION` needs to rethrow an exception (if any) using native exception handling mechanism instead of calling the new managed exception handling, as the native exception needs to propagate through some native code layers from there. This change adds parameter to the `UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE` macro to select whether to rethrow the exception as native or to invoke the new managed exception handling. This problem didn't show up until I ran the coreclr tests with tiered compilation disabled.

janvorli · 2023-08-22T15:29:32Z

@jkotas I believe I have addressed all of the feedback. Can you please take a look to see if you have any other comments or if it can be merged? The CI failures are unrelated.

jkotas · 2023-08-23T02:04:09Z

The UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE in the
EE_TO_JIT_TRANSITION needs to rethrow an exception (if any) using native
exception handling mechanism instead of calling the new managed
exception handling, as the native exception needs to propagate through
some native code layers from there.

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too. For example, this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.cpp#L6055-L6057

janvorli · 2023-08-23T13:01:04Z

Is JIT/EE interface really the only place that needs this change? I went over all UNINSTALL_UNWIND_AND_CONTINUE_HANDLER occurences and it seems that some of them need this fix too

That's a good point, I'll review the usage. The place that you've mentioned definitely needs it.

There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

janvorli added the area-ExceptionHandling-coreclr only use for closed issues label Jun 26, 2023

janvorli added this to the 8.0.0 milestone Jun 26, 2023

janvorli requested a review from jkotas June 26, 2023 09:49

janvorli requested a review from MichalStrehovsky as a code owner June 26, 2023 09:49

janvorli self-assigned this Jun 26, 2023

This was referenced Jun 26, 2023
Work item tests crash on OSX x64 Debug #87287
Closed
LibraryImportGenerator.Unit.Tests crashed in CI #87951
Closed

This was referenced Jun 26, 2023
Infra improvements for Helix #68176
Closed
Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040
Closed

jkotas reviewed Jun 26, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/proftoeeinterfaceimpl.cpp Outdated

jkotas reviewed Jun 26, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/frames.h Outdated

jkotas reviewed Jun 26, 2023
View reviewed changes

Comment threadsrc/coreclr/debug/daccess/dacdbiimplstackwalk.cpp Outdated

jkotas reviewed Jun 26, 2023
View reviewed changes

Comment threadsrc/coreclr/jit/codegencommon.cpp Outdated

jkotas requested a review from davidwrighton June 26, 2023 17:26

AustinWise reviewed Jun 26, 2023
View reviewed changes

Comment threadsrc/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs Outdated

janvorli requested a review from AaronRobinsonMSFT June 28, 2023 12:30

AaronRobinsonMSFT reviewed Jun 28, 2023
View reviewed changes

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch from a96c6a9 to 57a7e78Compare June 28, 2023 21:36

build-analysisBot mentioned this pull request Jun 29, 2023
windows-x86 release MSBuild_CMake is failing #88172
Closed

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch from 57a7e78 to 6db013dCompare June 29, 2023 07:30

jkotas reviewed Jun 29, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/fcall.h Outdated

jkotas reviewed Jul 25, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AppContext.cs Outdated

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch from c77f189 to ee3f998Compare July 26, 2023 21:05

davidwrighton reviewed Jul 26, 2023
View reviewed changes

build-analysisBot mentioned this pull request Jul 27, 2023
Tests.System.TimeProviderTests.TestProviderTimer test failure #87477
Closed

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch from ae7c358 to 3237ed2Compare July 28, 2023 15:04

build-analysisBot mentioned this pull request Jul 28, 2023
Microsoft.Extensions.SourceGeneration.Configuration.Binder.Tests.ConfigurationBinderTests.GetCanReadStaticProperty failed #88843
Closed

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch 2 times, most recently from bac0462 to dfc9c71Compare August 16, 2023 21:29

janvorli added 3 commits August 22, 2023 15:06

Remove Rhp prefix from the native helpers
278ba85

janvorli force-pushed the port-nativeaot-eh-to-coreclr-final-2 branch from c3824e1 to 28f603bCompare August 22, 2023 13:06

This was referenced Aug 22, 2023
System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783
Closed
tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource failing in CI #90605
Open

Update UNINSTALL_UNWIND_AND_CONTINUE_HANDLER usage
c4f2147
There were three places where the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER needed to be replaced by UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE(true).

jkotas reviewed Aug 24, 2023
View reviewed changes

Comment threadsrc/coreclr/vm/excep.cpp Outdated

jkotas approved these changes Aug 24, 2023
View reviewed changes

Rename INSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE
dafc94d
To INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX, as the old name is obsolete

This was referenced Aug 24, 2023
[Test Failure] System.Net.Security.Tests.CertificateValidationClientServer.CertificateValidationClientServer_EndToEnd_Ok on Windows AoT #88832
Closed
[8.0] [mono] Test process gets killed #90834
Closed

janvorli merged commit f8d9b3c into dotnet:mainAug 24, 2023

janvorli deleted the port-nativeaot-eh-to-coreclr-final-2 branch August 24, 2023 20:50

LuckyXu-HF added a commit to LuckyXu-HF/runtime that referenced this pull request Aug 25, 2023
[LoongArch64] Fix the compile error afterdotnet#88034.
5517d55

LuckyXu-HF mentioned this pull request Aug 25, 2023
[LoongArch64] Fix the compile error after #88034. #91109
Merged

janvorli pushed a commit that referenced this pull request Aug 25, 2023
[LoongArch64] Fix the compile error after#88034. (#91109)
71c68bf

jkotas mentioned this pull request Aug 29, 2023
[native aot] Unhandled exceptions are not printed to console #91298
Closed

ghost locked as resolved and limited conversation to collaborators Sep 24, 2023