Skip to content

Commit

Permalink
Make SPMI collections work on crashes/assertion failures (dotnet#69494)
Browse files Browse the repository at this point in the history
This changes SPMI to set up a PAL_TRY/PAL_FINALLY around the call to
ICorJitCompiler::compileMethod and save the method context in the finally. To
make this work we also need to pass COMPlus_JitThrowOnAssertionFailure=1 to
avoid the failfast so that the PAL_FINALLY is actually invoked.

To support hardware exceptions on non-Windows a little work was required:

* On non-Windows we were not passing an exports file when linking
  superpmi-shim-collector leading to exporting the entire PAL and tons of
  unnecessary functions. This would additionally mean we were calling
  PAL_InitializeDll in two places: on load, the coreclr PAL loader would invoke
  PAL_RegisterModule that does initialization, and then we would call it
  manually again from jitStartup. While the double initialization was ok it made
  reasoning about initialization difficult (and different from clrjit).

* We need to pass PAL_INITIALIZE_REGISTER_SIGNALS on initialization of the PAL
  make sure it properly attaches signal handlers that translates hardware
  exceptions to C++ exceptions. We cannot rely on the signal handler registered
  by coreclr as the different PALs have different CPalThread structures attached
  to the current thread.  This means that coreclr's signal handler will not
  translate hardware exceptions occurring in the JIT even when we have a PAL_TRY
  in superpmi-shim-collector that tries to enable hardware exceptions.  We need
  to define FEATURE_ENABLE_HARDWARE_EXCEPTIONS.

In addition I have changed the recording to record contexts indiscriminately,
even when we don't return CORJIT_OK.

With these changes I have verified that collections generated when there are
either JIT assertion failures or segfault/AV in JIT generates a collection that
reproduces the problem on Windows, Ubuntu and macOS.

Fixes dotnet#41649
  • Loading branch information
jakobbotsch authored May 26, 2022
1 parent 5734a23 commit 3436758
Show file tree
Hide file tree
Showing 10 changed files with 126 additions and 33 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/inc/clrconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ CONFIG_DWORD_INFO(INTERNAL_JitDebuggable, W("JitDebuggable"), 0, "")
#endif
RETAIL_CONFIG_DWORD_INFO(INTERNAL_JitEnableNoWayAssert, W("JitEnableNoWayAssert"), INTERNAL_JitEnableNoWayAssert_Default, "")
RETAIL_CONFIG_DWORD_INFO(UNSUPPORTED_JitFramed, W("JitFramed"), 0, "Forces EBP frames")
CONFIG_DWORD_INFO(INTERNAL_JitThrowOnAssertionFailure, W("JitThrowOnAssertionFailure"), 0, "Throw managed exception on assertion failures during JIT instead of crashing the process")
CONFIG_DWORD_INFO(INTERNAL_JitThrowOnAssertionFailure, W("JitThrowOnAssertionFailure"), 0, "Throw managed exception on assertion failures during JIT instead of failfast")
CONFIG_DWORD_INFO(INTERNAL_JitGCStress, W("JitGCStress"), 0, "GC stress mode for jit")
CONFIG_DWORD_INFO(INTERNAL_JitHeartbeat, W("JitHeartbeat"), 0, "")
CONFIG_DWORD_INFO(INTERNAL_JitHelperLogging, W("JitHelperLogging"), 0, "")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ void SpmiException::ShowAndDeleteMessage()
}
else
{
LogError("Unexpected exception was thrown.");
LogError("Unexpected exception %x was thrown.", exCode);
}

DeleteMessage();
Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/tools/superpmi/superpmi-shared/errorhandling.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ void MSC_ONLY(__declspec(noreturn)) ThrowException(DWORD exceptionCode, const ch
// Functions and types used by PAL_TRY-related macros.
//

extern bool IsSuperPMIException(unsigned code);

extern LONG FilterSuperPMIExceptions_CatchMC(PEXCEPTION_POINTERS pExceptionPointers, LPVOID lpvParam);

struct FilterSuperPMIExceptionsParam_CaptureException
Expand All @@ -114,12 +116,15 @@ struct FilterSuperPMIExceptionsParam_CaptureException
void Initialize(PEXCEPTION_POINTERS pExceptionPointers)
{
exceptionCode = pExceptionPointers->ExceptionRecord->ExceptionCode;
exceptionMessage = (pExceptionPointers->ExceptionRecord->NumberParameters != 1) ? nullptr : (char*)pExceptionPointers->ExceptionRecord->ExceptionInformation[0];
exceptionMessage = nullptr;
if (IsSuperPMIException(exceptionCode) && (pExceptionPointers->ExceptionRecord->NumberParameters >= 1))
{
exceptionMessage = (char*)pExceptionPointers->ExceptionRecord->ExceptionInformation[0];
}
}
};

extern LONG FilterSuperPMIExceptions_CaptureExceptionAndStop(PEXCEPTION_POINTERS pExceptionPointers, LPVOID lpvParam);

extern bool RunWithErrorTrap(void (*function)(void*), void* param);
extern bool RunWithSPMIErrorTrap(void (*function)(void*), void* param);
extern void RunWithErrorExceptionCodeCaptureAndContinueImp(void* param, void (*function)(void*), void (*finallyFunction)(void*, DWORD));
Expand Down
25 changes: 23 additions & 2 deletions src/coreclr/tools/superpmi/superpmi-shim-collector/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ remove_definitions(-D_UNICODE)

add_definitions(-DFEATURE_NO_HOST)
add_definitions(-DSELF_NO_HOST)
add_definitions(-DFEATURE_ENABLE_HARDWARE_EXCEPTIONS)

if(CLR_CMAKE_HOST_WIN32)
#use static crt
Expand Down Expand Up @@ -34,9 +35,24 @@ set(SUPERPMI_SHIM_COLLECTOR_SOURCES
../superpmi-shared/spmidumphelper.cpp
)
if (CLR_CMAKE_TARGET_WIN32)
preprocess_file(${CMAKE_CURRENT_SOURCE_DIR}/superpmi-shim-collector.def ${CMAKE_CURRENT_BINARY_DIR}/superpmi-shim-collector.def)
set(SPMI_COLLECTOR_EXPORTS ${CMAKE_CURRENT_SOURCE_DIR}/superpmi-shim-collector.def)
set(SPMI_COLLECTOR_EXPORTS_FINAL_FILE ${CMAKE_CURRENT_BINARY_DIR}/superpmi-shim-collector.def)
preprocess_file(${SPMI_COLLECTOR_EXPORTS} ${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})

list(APPEND SUPERPMI_SHIM_COLLECTOR_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/superpmi-shim-collector.def)
list(APPEND SUPERPMI_SHIM_COLLECTOR_SOURCES ${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})
else()
set(SPMI_COLLECTOR_EXPORTS ${CMAKE_CURRENT_SOURCE_DIR}/superpmi-shim-collector.PAL.exports)
set(SPMI_COLLECTOR_EXPORTS_FINAL_FILE ${CMAKE_CURRENT_BINARY_DIR}/superpmi-shim-collector.PAL.exports)

generate_exports_file(${SPMI_COLLECTOR_EXPORTS} ${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})

if(CLR_CMAKE_TARGET_LINUX OR CLR_CMAKE_TARGET_FREEBSD OR CLR_CMAKE_TARGET_NETBSD OR CLR_CMAKE_TARGET_SUNOS)
# This is required to force using our own PAL, not one that we are loaded with.
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic")
endif()

set_exports_linker_option(${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})
set(SPMI_COLLECTOR_EXPORTS_LINKER_OPTION ${EXPORTS_LINKER_OPTION})
endif (CLR_CMAKE_TARGET_WIN32)

add_library_clr(superpmi-shim-collector
Expand All @@ -46,6 +62,11 @@ add_library_clr(superpmi-shim-collector

target_precompile_headers(superpmi-shim-collector PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:standardpch.h>")

add_custom_target(spmi_exports DEPENDS ${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})
add_dependencies(superpmi-shim-collector spmi_exports)
set_property(TARGET superpmi-shim-collector APPEND_STRING PROPERTY LINK_FLAGS ${SPMI_COLLECTOR_EXPORTS_LINKER_OPTION})
set_property(TARGET superpmi-shim-collector APPEND_STRING PROPERTY LINK_DEPENDS ${SPMI_COLLECTOR_EXPORTS_FINAL_FILE})

if(CLR_CMAKE_HOST_UNIX)
target_link_libraries(superpmi-shim-collector
utilcodestaticnohost
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,32 @@ void interceptor_ICJC::setTargetOS(CORINFO_OS os)
original_ICorJitCompiler->setTargetOS(os);
}

void interceptor_ICJC::finalizeAndCommitCollection(MethodContext* mc, CorJitResult result, uint8_t* nativeEntry, uint32_t nativeSizeOfCode)
{
mc->cr->recCompileMethod(&nativeEntry, &nativeSizeOfCode, result);

if (result == CORJIT_OK)
{
mc->cr->recAllocMemCapture();
mc->cr->recAllocGCInfoCapture();
}

mc->saveToFile(hFile);
}

CorJitResult interceptor_ICJC::compileMethod(ICorJitInfo* comp, /* IN */
struct CORINFO_METHOD_INFO* info, /* IN */
unsigned /* code:CorJitFlag */ flags, /* IN */
uint8_t** nativeEntry, /* OUT */
uint32_t* nativeSizeOfCode /* OUT */
)
{
interceptor_ICJI our_ICorJitInfo;
our_ICorJitInfo.original_ICorJitInfo = comp;

auto* mc = new MethodContext();
our_ICorJitInfo.mc = mc;
our_ICorJitInfo.mc->cr->recProcessName(GetCommandLineA());
interceptor_ICJI our_ICorJitInfo(this, comp, mc);

our_ICorJitInfo.mc->recCompileMethod(info, flags, currentOs);
mc->cr->recProcessName(GetCommandLineA());

mc->recCompileMethod(info, flags, currentOs);

// force some extra data into our tables..
// data probably not needed with RyuJIT, but needed in 4.5 and 4.5.1 to help with catching cached values
Expand All @@ -57,25 +68,57 @@ CorJitResult interceptor_ICJC::compileMethod(ICorJitInfo* comp,
// Record data from the global context, if any
if (g_globalContext != nullptr)
{
our_ICorJitInfo.mc->recGlobalContext(*g_globalContext);
mc->recGlobalContext(*g_globalContext);
}

CorJitResult temp =
original_ICorJitCompiler->compileMethod(&our_ICorJitInfo, info, flags, nativeEntry, nativeSizeOfCode);

if (temp == CORJIT_OK)
struct CompileParams
{
// capture the results of compilation
our_ICorJitInfo.mc->cr->recCompileMethod(nativeEntry, nativeSizeOfCode, temp);

our_ICorJitInfo.mc->cr->recAllocMemCapture();
our_ICorJitInfo.mc->cr->recAllocGCInfoCapture();
our_ICorJitInfo.mc->saveToFile(hFile);
}

delete mc;

return temp;
ICorJitCompiler* origComp;
interceptor_ICJI* ourICJI;
struct CORINFO_METHOD_INFO* methodInfo;
unsigned flags;
uint8_t** nativeEntry;
uint32_t* nativeSizeOfCode;
CorJitResult result;
} compileParams;

compileParams.origComp = original_ICorJitCompiler;
compileParams.ourICJI = &our_ICorJitInfo;
compileParams.methodInfo = info;
compileParams.flags = flags;
compileParams.nativeEntry = nativeEntry;
compileParams.nativeSizeOfCode = nativeSizeOfCode;
compileParams.result = CORJIT_INTERNALERROR;

*nativeEntry = nullptr;
*nativeSizeOfCode = 0;

auto doCompile = [mc, our_ICorJitInfo, this, &compileParams]()
{
PAL_TRY(CompileParams*, pParam, &compileParams)
{
pParam->result = pParam->origComp->compileMethod(
pParam->ourICJI,
pParam->methodInfo,
pParam->flags,
pParam->nativeEntry,
pParam->nativeSizeOfCode);
}
PAL_FINALLY
{
if (!our_ICorJitInfo.SavedCollectionEarly())
{
finalizeAndCommitCollection(mc, compileParams.result, *compileParams.nativeEntry, *compileParams.nativeSizeOfCode);
}

delete mc;
}
PAL_ENDTRY;
};

doCompile();

return compileParams.result;
}

void interceptor_ICJC::ProcessShutdownWork(ICorStaticInfo* info)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "runtimedetails.h"

class MethodContext;

class interceptor_ICJC : public ICorJitCompiler
{

Expand All @@ -16,6 +18,8 @@ class interceptor_ICJC : public ICorJitCompiler
ICorJitCompiler* original_ICorJitCompiler;
HANDLE hFile;
CORINFO_OS currentOs;

void finalizeAndCommitCollection(MethodContext* mc, CorJitResult result, uint8_t* nativeEntry, uint32_t nativeSizeOfCode);
};

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -1937,6 +1937,12 @@ bool interceptor_ICJI::logMsg(unsigned level, const char* fmt, va_list args)
int interceptor_ICJI::doAssert(const char* szFile, int iLine, const char* szExpr)
{
mc->cr->AddCall("doAssert");

m_compiler->finalizeAndCommitCollection(mc, CORJIT_INTERNALERROR, nullptr, 0);
// The following assert may not always fail fast, so make sure we do not
// save the collection twice if it throws an unwindable exception.
m_savedCollectionEarly = true;

return original_ICorJitInfo->doAssert(szFile, iLine, szExpr);
}

Expand Down
20 changes: 15 additions & 5 deletions src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,29 @@
#include "runtimedetails.h"
#include "methodcontext.h"

class interceptor_ICJC;

class interceptor_ICJI : public ICorJitInfo
{

#include "icorjitinfoimpl.h"

private:
void makeFatMC_ClassHandle(CORINFO_CLASS_HANDLE cls, bool getAttribs);
interceptor_ICJC* m_compiler;
ICorJitInfo* original_ICorJitInfo;
MethodContext* mc;
bool m_savedCollectionEarly;

public:
// Added to help us track the original icji and be able to easily indirect
// to it. And a simple way to keep one memory manager instance per instance.
ICorJitInfo* original_ICorJitInfo;
MethodContext* mc;
interceptor_ICJI(interceptor_ICJC* compiler, ICorJitInfo* original, MethodContext* mc)
: m_compiler(compiler)
, original_ICorJitInfo(original)
, mc(mc)
, m_savedCollectionEarly(false)
{
}

bool SavedCollectionEarly() const { return m_savedCollectionEarly; }
};

#endif
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
getJit
jitStartup
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ void InitializeShim()
}

#ifdef HOST_UNIX
// Register signal handlers for the shim so we can handle committing collections on JIT segfaults.
PAL_SetInitializeDLLFlags(PAL_INITIALIZE_REGISTER_SIGNALS);
if (0 != PAL_InitializeDLL())
{
fprintf(stderr, "Error: Fail to PAL_InitializeDLL\n");
Expand Down

0 comments on commit 3436758

Please sign in to comment.