Skip to content

Commit

Permalink
Improve debugger StackOverflow handling (#108172)
Browse files Browse the repository at this point in the history
* Improve debugger StackOverflow handling

In the function Debugger::IsThreadAtSafePlace we have some code that attempts to avoid calling a stack intensive helper method if the runtime is currently handling a StackOverflowException. The logic to do that check was incorrect because the runtime's EH has some special case handling for StackOverflow. Most exceptions cause the EH code to create an ExceptionTracker object which in turn causes thread->IsExceptionInProgress() to return TRUE. StackOverflow doesn't set that and goes straight to fatal error handling so thread->IsExceptionInProgress() is false.

The fix avoids checking IsExceptionInProgress(). g_pEEInterface->GetThreadException() works because it checks both the tracker and also falls back to thread->m_lastThrownObject where the StackOverflow exception will be stored.

Fixes #70755

Co-authored-by: mikelle-rogers <[email protected]>
  • Loading branch information
noahfalk and mikelle-rogers authored Sep 24, 2024
1 parent ffcc15b commit ee53b27
Showing 1 changed file with 6 additions and 11 deletions.
17 changes: 6 additions & 11 deletions src/coreclr/debug/ee/debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12595,21 +12595,16 @@ bool Debugger::IsThreadAtSafePlace(Thread *thread)
return true;
}

// <TODO>
//
// Make sure this fix is evaluated when doing real work for debugging SO handling.
//
// On the Stack Overflow code path calling IsThreadAtSafePlaceWorker as it is
// currently implemented is way too stack intensive. For now we cheat and just
// say that if a thread is in the middle of handling a SO it is NOT at a safe
// place. This is a reasonably safe assumption to make and hopefully shouldn't
// result in deadlocking the debugger.
if ( (thread->IsExceptionInProgress()) &&
(g_pEEInterface->GetThreadException(thread) == CLRException::GetPreallocatedStackOverflowExceptionHandle()) )
// currently implemented is way too stack intensive. Conservatively we assume that
// any thread handling a SO is not at a safe place.
// NOTE: don't check for thread->IsExceptionInProgress(), SO has special handling
// that directly sets the last thrown object without ever creating a tracker.
// (Tracker is what thread->IsExceptionInProgress() checks for)
if (g_pEEInterface->GetThreadException(thread) == CLRException::GetPreallocatedStackOverflowExceptionHandle())
{
return false;
}
// </TODO>
else
{
return IsThreadAtSafePlaceWorker(thread);
Expand Down

0 comments on commit ee53b27

Please sign in to comment.