-
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
Adjust IP after an AV during an STEP with FEATURE_EMULATE_SINGLESTEP enabled #105633
Conversation
@@ -262,6 +273,8 @@ SEHProcessException(PAL_SEHException* exception) | |||
_ASSERTE(g_safeExceptionCheckFunction != NULL); | |||
// Check if it is safe to handle the hardware exception (the exception happened in managed code | |||
// or in a jitter helper or it is a debugger breakpoint) | |||
if (g_handleSingleStepAfterExceptionHandler) |
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.
Rather than adding a new handler, could we merge the fixup logic into the g_safeExceptionCheckFunction? It looks like other fixups are already being applied in that function:
runtime/src/coreclr/vm/exceptionhandling.cpp
Line 5433 in 8839704
// Pretend we were executing the barrier function at its original location |
Not required, just figured it might cut down needing to setup and define new callbacks.
src/coreclr/vm/exceptionhandling.cpp
Outdated
pThread->HandleSingleStep(contextRecord, exceptionRecord->ExceptionCode); | ||
return TRUE; | ||
} | ||
return FALSE; |
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.
This function returns TRUE/FALSE, but the callsite ignores the value. Any reason to have it if we aren't using it?
BOOL HandleSingleStepAfterException(PCONTEXT contextRecord, PEXCEPTION_RECORD exceptionRecord) | ||
{ | ||
Thread *pThread = GetThreadNULLOk(); | ||
if (pThread && pThread->IsSingleStepEnabled() && |
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.
Any reason why we only invoke pThread->HandleSingleStep() in some cases and not others? Could we do something like this in IsSafeToHandleHardwareException() and remove the HandleSingleStep call from HandleHardwareException?
#ifdef FEATURE_EMULATE_SINGLESTEP
Thread *pThread = GetThreadNULLOk();
if (pThread != NULL && g_pDebugInterface != NULL)
{
HandleSingleStep(contextRecord, exceptionRecord, pThread);
}
#endif
It feels easier to understand if we always invoke it from one place rather than conditionally invoke it from two different place.
exceptionRecord->ExceptionCode != STATUS_SINGLE_STEP && | ||
exceptionRecord->ExceptionCode != STATUS_STACK_OVERFLOW) | ||
{ | ||
pThread->HandleSingleStep(contextRecord, exceptionRecord->ExceptionCode); |
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.
The updated version of the change looks like it still calls HandleSingleStep from two different places, once here and once over in HandleHardwareException.
I was suggesting that we converge those into a single call here. Is there a reason we need two different calls? If yes it would be great to add a comment saying why we have both, and if not it would be nice to get rid of the HandleHardwareException portion and do it all here.
Fix step in a line that will throw an AV
Without this PR in this function
IsSafeToHandleHardwareException
this other functionExecutionManager::IsManagedCode(controlPc)
was returning FALSE, basically because we were trying to find the patched IP in the Jitted code and then it wasn't found, right now I'm adjusting the patched IP to the original IP before trying to check the is it's in a managed code.Fixes #95189