Skip to content

Improve windows hardware exception handling performance #74426

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

Merged
merged 4 commits into from
Aug 24, 2022
Merged

Improve windows hardware exception handling performance #74426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab0f74a
Improve windows hardware exception handling performance
janvorli Aug 23, 2022
4f65ae0
Fix x86 build
janvorli Aug 23, 2022
0031316
Reflect PR feedback nit
janvorli Aug 23, 2022
f8e169c
Use proper type of local for CLRVectoredExceptionHandler at one place
janvorli Aug 24, 2022
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
96 changes: 42 additions & 54 deletions src/coreclr/vm/excep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6723,27 +6723,16 @@ bool ShouldHandleManagedFault(

#ifndef TARGET_UNIX

LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo);

enum VEH_ACTION
{
VEH_NO_ACTION = 0,
VEH_EXECUTE_HANDLE_MANAGED_EXCEPTION,
VEH_CONTINUE_EXECUTION,
VEH_CONTINUE_SEARCH,
VEH_EXECUTE_HANDLER
};


VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo);
VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase3(PEXCEPTION_POINTERS pExceptionInfo);

LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
VEH_ACTION WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
{
// It is not safe to execute code inside VM after we shutdown EE. One example is DisablePreemptiveGC
// will block forever.
if (g_fForbidEnterEE)
{
return EXCEPTION_CONTINUE_SEARCH;
return VEH_CONTINUE_SEARCH;
}


Expand Down Expand Up @@ -6833,7 +6822,7 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
pExceptionInfo->ContextRecord->Rip = hijackArgs.ReturnAddress;
}

return EXCEPTION_CONTINUE_EXECUTION;
return VEH_CONTINUE_EXECUTION;
}
#endif

Expand All @@ -6857,11 +6846,9 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
//
// Not an Out-of-memory situation, so no need for a forbid fault region here
//
return EXCEPTION_CONTINUE_SEARCH;
return VEH_CONTINUE_SEARCH;
}

LONG retVal = 0;

// We can't probe here, because we won't return from the CLRVectoredExceptionHandlerPhase2
// on WIN64
//
Expand All @@ -6872,15 +6859,10 @@ LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
CantAllocHolder caHolder;
}

retVal = CLRVectoredExceptionHandlerPhase2(pExceptionInfo);

//
//END_ENTRYPOINT_VOIDRET;
//
return retVal;
return CLRVectoredExceptionHandlerPhase2(pExceptionInfo);
}

LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo)
VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo)
{
//
// DO NOT USE CONTRACTS HERE AS THIS ROUTINE MAY NEVER RETURN. You can use
Expand Down Expand Up @@ -6914,27 +6896,16 @@ LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo
action = CLRVectoredExceptionHandlerPhase3(pExceptionInfo);
}

if (action == VEH_CONTINUE_EXECUTION)
if ((action == VEH_CONTINUE_EXECUTION) || (action == VEH_CONTINUE_SEARCH) || (action == VEH_EXECUTE_HANDLER))
{
return EXCEPTION_CONTINUE_EXECUTION;
}

if (action == VEH_CONTINUE_SEARCH)
{
return EXCEPTION_CONTINUE_SEARCH;
}

if (action == VEH_EXECUTE_HANDLER)
{
return EXCEPTION_EXECUTE_HANDLER;
return action;
}

#if defined(FEATURE_EH_FUNCLETS)

if (action == VEH_EXECUTE_HANDLE_MANAGED_EXCEPTION)
{
HandleManagedFault(pExceptionInfo->ExceptionRecord, pExceptionInfo->ContextRecord);
return EXCEPTION_CONTINUE_EXECUTION;
return action;
}

#endif // defined(FEATURE_EH_FUNCLETS)
Expand All @@ -6954,7 +6925,7 @@ LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo
// the choice to break the no-trigger region after taking all necessary precautions.
if (IsDebuggerFault(pExceptionRecord, pExceptionInfo->ContextRecord, pExceptionRecord->ExceptionCode, GetThreadNULLOk()))
{
return EXCEPTION_CONTINUE_EXECUTION;
return VEH_CONTINUE_EXECUTION;
}
}

Expand Down Expand Up @@ -6986,11 +6957,11 @@ LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo
{
// The breakpoint was not ours. Someone else can handle it. (Or if not, we'll get it again as
// an unhandled exception.)
return EXCEPTION_CONTINUE_SEARCH;
return VEH_CONTINUE_SEARCH;
}

// The breakpoint was from managed or the runtime. Handle it.
return UserBreakpointFilter(pExceptionInfo);
return (VEH_ACTION)UserBreakpointFilter(pExceptionInfo);
}

#if defined(FEATURE_EH_FUNCLETS)
Expand All @@ -7008,15 +6979,11 @@ LONG WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExceptionInfo

if (fShouldHandleManagedFault)
{
//
// HandleManagedFault may never return, so we cannot use a forbid fault region around it.
//
HandleManagedFault(pExceptionInfo->ExceptionRecord, pExceptionInfo->ContextRecord);
return EXCEPTION_CONTINUE_EXECUTION;
}
return VEH_EXECUTE_HANDLE_MANAGED_EXCEPTION;
}
#endif // defined(FEATURE_EH_FUNCLETS)

return EXCEPTION_EXECUTE_HANDLER;
return VEH_EXECUTE_HANDLER;
}

/*
Expand Down Expand Up @@ -7583,13 +7550,34 @@ LONG WINAPI CLRVectoredExceptionHandlerShim(PEXCEPTION_POINTERS pExceptionInfo)
if (pThread || fExceptionInEE)
{
if (!bIsGCMarker)
result = CLRVectoredExceptionHandler(pExceptionInfo);
else
result = EXCEPTION_CONTINUE_EXECUTION;
{
VEH_ACTION action = CLRVectoredExceptionHandler(pExceptionInfo);

#ifdef FEATURE_EH_FUNCLETS
if (VEH_EXECUTE_HANDLE_MANAGED_EXCEPTION == action)
{
//
// HandleManagedFault may never return, so we cannot use a forbid fault region around it.
//
HandleManagedFault(pExceptionInfo->ExceptionRecord, pExceptionInfo->ContextRecord);
return EXCEPTION_CONTINUE_EXECUTION;
}
#endif // FEATURE_EH_FUNCLETS

if (EXCEPTION_EXECUTE_HANDLER == result)
if (VEH_EXECUTE_HANDLER == action)
{
result = EXCEPTION_CONTINUE_SEARCH;
}
else
{
_ASSERTE((action == VEH_CONTINUE_EXECUTION) || (action == VEH_CONTINUE_SEARCH));
result = (LONG)action;
}

}
else
{
result = EXCEPTION_CONTINUE_SEARCH;
result = EXCEPTION_CONTINUE_EXECUTION;
}

#ifdef _DEBUG
Expand Down
11 changes: 10 additions & 1 deletion src/coreclr/vm/exceptmacros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,16 @@ VOID DECLSPEC_NORETURN RealCOMPlusThrowOM();

#endif // !defined(FEATURE_EH_FUNCLETS)

LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo);
enum VEH_ACTION
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these really need to be in the header? Adding definitions to the header implies they are used in other places whereas this are not.

Copy link
Member Author

@janvorli janvorli Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AaronRobinsonMSFT, it is actually used in two sources, the excep.cpp and excepx86.cpp. However, I have forgotten to modify the excepx86.cpp to get the result of the CLRVectoredExceptionHandler call into VEH_ACTION, it was still using DWORD. I am updating the change.

{
VEH_NO_ACTION = -3,
VEH_EXECUTE_HANDLE_MANAGED_EXCEPTION = -2,
VEH_CONTINUE_EXECUTION = EXCEPTION_CONTINUE_EXECUTION,
VEH_CONTINUE_SEARCH = EXCEPTION_CONTINUE_SEARCH,
VEH_EXECUTE_HANDLER = EXCEPTION_EXECUTE_HANDLER
};

VEH_ACTION CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo);

// Actual UEF worker prototype for use by GCUnhandledExceptionFilter.
extern LONG InternalUnhandledExceptionFilter_Worker(PEXCEPTION_POINTERS pExceptionInfo);
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/i386/excepx86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,13 +1254,13 @@ CPFH_FirstPassHandler(EXCEPTION_RECORD *pExceptionRecord,
// Call to the vectored handler to give other parts of the Runtime a chance to jump in and take over an
// exception before we do too much with it. The most important point in the vectored handler is not to toggle
// the GC mode.
DWORD filter = CLRVectoredExceptionHandler(&ptrs);
VEH_ACTION filter = CLRVectoredExceptionHandler(&ptrs);

if (filter == (DWORD) EXCEPTION_CONTINUE_EXECUTION)
if (filter == VEH_CONTINUE_EXECUTION)
{
return ExceptionContinueExecution;
}
else if (filter == EXCEPTION_CONTINUE_SEARCH)
else if (filter == VEH_CONTINUE_SEARCH)
{
return ExceptionContinueSearch;
}
Expand Down