Skip to content

Debugger should not manage exceptions during shutdown #105270

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+6 −0
Closed

Debugger should not manage exceptions during shutdown #105270

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c09516d
Debugger should not manage exceptions during shutdown
mikelle-rogers Jul 22, 2024
eb56cd7
Merge branch 'main' into dev/mirogers/deadlock_shutdowm
mikelle-rogers Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5466,6 +5466,12 @@ bool Debugger::FirstChanceNativeException(EXCEPTION_RECORD *exception,
CONTRACTL_END;


if ((g_fEEShutDown & ShutDown_Finalize2) == ShutDown_Finalize2)
Copy link
Member

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?

Copy link
Member

@noahfalk noahfalk Jul 22, 2024 •
edited
Loading

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:

  1. Skip over the breakpoint as normal but potentially forego delivering the cross-process debugger notification.
  2. Remove all breakpoint patches at an earlier point in shutdown so that no exception is generated in the first place.

Copy link
Member

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.

Copy link
Member

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.

{
// We are shutting down the EE and don't want the debugger to manage any exceptions.
return false;
}

// Ignore any notification exceptions sent from code:Debugger.SendRawEvent.
// This is not a common case, but could happen in some cases described
// in SendRawEvent. Either way, Left-Side and VM should just ignore these.
Expand Down