-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debugger should not manage exceptions during shutdown #105270
Debugger should not manage exceptions during shutdown #105270
Conversation
@@ -5466,6 +5466,12 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception, | |||
CONTRACTL_END; | |||
|
|||
|
|||
if ((g_fEEShutDown & ShutDown_Finalize2) == ShutDown_Finalize2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this behave if the case that triggered this is an int3/bp that we set? Will we race to crash with the runtime that's shutting down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with @hoyosjs - in cases where the debugger set a breakpoint by overwriting a pre-existing instruction ignoring the OS notification probably results in a crash, hang, or executing unintended assembly instructions. Two potential solutions:
- Skip over the breakpoint as normal but potentially forego delivering the cross-process debugger notification.
- Remove all breakpoint patches at an earlier point in shutdown so that no exception is generated in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skip over the breakpoint as normal but potentially forego delivering the cross-process debugger notification.
I think this is the best way to fix the issue.
Remove all breakpoint patches at an earlier point in shutdown so that no exception is generated in the first place
We would need to ensure that no threads are executing the patches before proceeding. To do that, we would need to suspend the runtime that can come with a new set of issues. In general, we try to minimize amount of code that runs during shutdown. We just try to do the absolute minimum required to allow shutdown to progress. The first option fits the bill better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the first option, having two was mostly to afford some flexibility in case one ran into unforeseen issues. Agreed that the 2nd option requires more deliberate work in the shutdown code path.
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
When we are in the process of shutting down the EE, we do not want the debugger to manage any exceptions. The EE should continue to shut down and the debugger should ignore the exception.
Fixes #90079