From 16652fadd482f54ed55bd1a9c0da2f1c324a809b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 7 Jan 2022 00:42:26 +0100 Subject: [PATCH 1/4] Fix exception propagation over HW exception frame on macOS arm64 There is a problem unwinding over the PAL_DispatchExceptionWrapper to the actual hardware exception location. The unwinder is unable to get distinct LR and PC in that frame and sets both of them to the same value. This is caused by the fact that the PAL_DispatchExceptionWrapper is just an injected fake frame and there was no real call. Calls always return with LR and PC set to the same value. The fix unifies the hardware exception frame unwinding with Linux where we had problems unwinding over signal handler trampoline, so PAL_VirtualUnwind skips the trampoline and now also the PAL_DispatchExceptionWrapper frame by copying the context of the exception as the unwound context. --- .../pal/src/exception/machexception.cpp | 30 ++-------- src/coreclr/pal/src/exception/seh-unwind.cpp | 16 ++--- src/coreclr/pal/src/exception/signal.cpp | 7 +-- src/coreclr/pal/src/include/pal/seh.hpp | 5 ++ .../coreclr/GitHub_62058/test62058.cs | 60 +++++++++++++++++++ .../coreclr/GitHub_62058/test62058.csproj | 9 +++ 6 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 src/tests/Regressions/coreclr/GitHub_62058/test62058.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_62058/test62058.csproj diff --git a/src/coreclr/pal/src/exception/machexception.cpp b/src/coreclr/pal/src/exception/machexception.cpp index e5aebdf652c6a5..db4af62dabc5e1 100644 --- a/src/coreclr/pal/src/exception/machexception.cpp +++ b/src/coreclr/pal/src/exception/machexception.cpp @@ -369,34 +369,16 @@ void PAL_DispatchException(PCONTEXT pContext, PEXCEPTION_RECORD pExRecord, MachE { CPalThread *pThread = InternalGetCurrentThread(); - CONTEXT *contextRecord; - EXCEPTION_RECORD *exceptionRecord; - AllocateExceptionRecords(&exceptionRecord, &contextRecord); + CONTEXT *contextRecord = pContext; + g_hardware_exception_context_locvar_offset = (int)((char*)&contextRecord - (char*)__builtin_frame_address(0)); - *contextRecord = *pContext; - *exceptionRecord = *pExRecord; + pContext->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE; - contextRecord->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE; - bool continueExecution; + PAL_SEHException exception(pExRecord, pContext, true); - { - // The exception object takes ownership of the exceptionRecord and contextRecord - PAL_SEHException exception(exceptionRecord, contextRecord); - - TRACE("PAL_DispatchException(EC %08x EA %p)\n", pExRecord->ExceptionCode, pExRecord->ExceptionAddress); - - continueExecution = SEHProcessException(&exception); - if (continueExecution) - { - // Make a copy of the exception records so that we can free them before restoring the context - *pContext = *contextRecord; - *pExRecord = *exceptionRecord; - } - - // The exception records are destroyed by the PAL_SEHException destructor now. - } + TRACE("PAL_DispatchException(EC %08x EA %p)\n", pExRecord->ExceptionCode, pExRecord->ExceptionAddress); - if (continueExecution) + if (SEHProcessException(&exception)) { #if defined(HOST_ARM64) // RtlRestoreContext assembly corrupts X16 & X17, so it cannot be diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index 750f19109fbaa7..3091650ec5faa1 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -496,7 +496,9 @@ void GetContextPointers(unw_cursor_t *cursor, unw_context_t *unwContext, KNONVOL #ifndef HOST_WINDOWS -extern int g_common_signal_handler_context_locvar_offset; +// Frame pointer relative offset of a local containing a pointer to the windows style context of a location +// where a hardware exception occured. +int g_hardware_exception_context_locvar_offset = 0; BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextPointers) { @@ -506,19 +508,17 @@ BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextP DWORD64 curPc = CONTEXTGetPC(context); -#ifndef __APPLE__ - // Check if the PC is the return address from the SEHProcessException in the common_signal_handler. - // If that's the case, extract its local variable containing the windows style context of the hardware + // Check if the PC is the return address from the SEHProcessException. + // If that's the case, extract its local variable containing a pointer to the windows style context of the hardware // exception and return that. This skips the hardware signal handler trampoline that the libunwind - // cannot cross on some systems. + // cannot cross on some systems. On macOS, it skips a similar trampoline we create in HijackFaultingThread. if ((void*)curPc == g_SEHProcessExceptionReturnAddress) { - CONTEXT* signalContext = (CONTEXT*)(CONTEXTGetFP(context) + g_common_signal_handler_context_locvar_offset); - memcpy_s(context, sizeof(CONTEXT), signalContext, sizeof(CONTEXT)); + CONTEXT* exceptionContext = *(CONTEXT**)(CONTEXTGetFP(context) + g_hardware_exception_context_locvar_offset); + memcpy_s(context, sizeof(CONTEXT), exceptionContext, sizeof(CONTEXT)); return TRUE; } -#endif if ((context->ContextFlags & CONTEXT_EXCEPTION_ACTIVE) != 0) { diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 58fc78ef357e84..63b7ef115f26d1 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -116,10 +116,6 @@ struct sigaction g_previous_sigabrt; #if !HAVE_MACH_EXCEPTIONS -// Offset of the local variable containing pointer to windows style context in the common_signal_handler function. -// This offset is relative to the frame pointer. -int g_common_signal_handler_context_locvar_offset = 0; - // TOP of special stack for handling stack overflow volatile void* g_stackOverflowHandlerStack = NULL; @@ -942,11 +938,12 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext #if !HAVE_MACH_EXCEPTIONS sigset_t signal_set; CONTEXT signalContextRecord; + CONTEXT* signalContextRecordPtr = &signalContextRecord; EXCEPTION_RECORD exceptionRecord; native_context_t *ucontext; ucontext = (native_context_t *)sigcontext; - g_common_signal_handler_context_locvar_offset = (int)((char*)&signalContextRecord - (char*)__builtin_frame_address(0)); + g_hardware_exception_context_locvar_offset = (int)((char*)&signalContextRecordPtr - (char*)__builtin_frame_address(0)); if (code == (SIGSEGV | StackOverflowFlag)) { diff --git a/src/coreclr/pal/src/include/pal/seh.hpp b/src/coreclr/pal/src/include/pal/seh.hpp index 6ad89df0fdd650..327fe0d7fb03e2 100644 --- a/src/coreclr/pal/src/include/pal/seh.hpp +++ b/src/coreclr/pal/src/include/pal/seh.hpp @@ -145,5 +145,10 @@ CorUnix::PAL_ERROR SEHDisable(CorUnix::CPalThread *pthrCurrent); } +// Offset of the local variable containing pointer to windows style context in the common_signal_handler / PAL_DispatchException function. +// This offset is relative to the frame pointer. +extern int g_hardware_exception_context_locvar_offset; + + #endif /* _PAL_SEH_HPP_ */ diff --git a/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs b/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs new file mode 100644 index 00000000000000..3744007d9e45ac --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs @@ -0,0 +1,60 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +public class Program +{ + private interface IFoo + { + bool IsValid { get; } + } + + private class Foo : IFoo + { + public bool IsValid { get; set; } + } + + public static int Main(string[] args) + { + bool warmup = new Foo().IsValid; + CatchIgnore(() => + CatchRethrow(() => + { + IFoo[] foos = {new Foo(), null}; + foreach (var foo in foos) + { + bool check = foo.IsValid; + } + })); + + return 100; + } + + public static void CatchRethrow(Action action) + { + try + { + action.Invoke(); + } + catch (Exception e) + { + Console.Out.WriteLine("catch"); + Console.Out.Flush(); + throw new Exception("catch", e); // throw; doesn't work either + } + } + + public static void CatchIgnore(Action action) + { + try + { + action.Invoke(); + } + catch (Exception) + { + Console.Out.WriteLine("ignore"); + Console.Out.Flush(); + } + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_62058/test62058.csproj b/src/tests/Regressions/coreclr/GitHub_62058/test62058.csproj new file mode 100644 index 00000000000000..0fce5a0556f40e --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_62058/test62058.csproj @@ -0,0 +1,9 @@ + + + Exe + 1 + + + + + From f15b34e7f969bf85e910801a9a07e6cccab6a173 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jan 2022 22:11:42 +0100 Subject: [PATCH 2/4] Fix GC stress C - wrong context being restored The context that is restored in the PAL_DispatchException needs to be the one from the exception, not the original saved one. That ensures that the registers updated by the GC in GC stress C in the context are properly restored after the execution is resumed. --- src/coreclr/pal/src/exception/machexception.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/pal/src/exception/machexception.cpp b/src/coreclr/pal/src/exception/machexception.cpp index db4af62dabc5e1..5d3cd8846d9522 100644 --- a/src/coreclr/pal/src/exception/machexception.cpp +++ b/src/coreclr/pal/src/exception/machexception.cpp @@ -383,9 +383,9 @@ void PAL_DispatchException(PCONTEXT pContext, PEXCEPTION_RECORD pExRecord, MachE #if defined(HOST_ARM64) // RtlRestoreContext assembly corrupts X16 & X17, so it cannot be // used for GCStress=C restore - MachSetThreadContext(pContext); + MachSetThreadContext(exception.ExceptionPointers.ContextRecord); #else - RtlRestoreContext(pContext, pExRecord); + RtlRestoreContext(exception.ExceptionPointers.ContextRecord, pExRecord); #endif } From cc2df9ccf3183ab76fda534822660001a3018853 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 14 Jan 2022 17:07:20 +0100 Subject: [PATCH 3/4] Fix exception context leak in GC stress C The PAL_SEHException had the records allocated on stack, so the direct context restoration after the EH for GC stress C completed leaked those. --- .../pal/src/exception/machexception.cpp | 22 ++++++++++++++----- .../coreclr/GitHub_62058/test62058.cs | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/coreclr/pal/src/exception/machexception.cpp b/src/coreclr/pal/src/exception/machexception.cpp index 5d3cd8846d9522..eca89e0a204c64 100644 --- a/src/coreclr/pal/src/exception/machexception.cpp +++ b/src/coreclr/pal/src/exception/machexception.cpp @@ -373,19 +373,31 @@ void PAL_DispatchException(PCONTEXT pContext, PEXCEPTION_RECORD pExRecord, MachE g_hardware_exception_context_locvar_offset = (int)((char*)&contextRecord - (char*)__builtin_frame_address(0)); pContext->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE; + bool continueExecution; + { + PAL_SEHException exception(pExRecord, pContext, true); - PAL_SEHException exception(pExRecord, pContext, true); + TRACE("PAL_DispatchException(EC %08x EA %p)\n", pExRecord->ExceptionCode, pExRecord->ExceptionAddress); - TRACE("PAL_DispatchException(EC %08x EA %p)\n", pExRecord->ExceptionCode, pExRecord->ExceptionAddress); + continueExecution = SEHProcessException(&exception); + if (continueExecution) + { + // Make a copy of the exception records so that we can free them before restoring the context + *pContext = *exception.ExceptionPointers.ContextRecord; + *pExRecord = *exception.ExceptionPointers.ExceptionRecord; + } + + // The exception records are destroyed by the PAL_SEHException destructor now. + } - if (SEHProcessException(&exception)) + if (continueExecution) { #if defined(HOST_ARM64) // RtlRestoreContext assembly corrupts X16 & X17, so it cannot be // used for GCStress=C restore - MachSetThreadContext(exception.ExceptionPointers.ContextRecord); + MachSetThreadContext(pContext); #else - RtlRestoreContext(exception.ExceptionPointers.ContextRecord, pExRecord); + RtlRestoreContext(pContext, pExRecord); #endif } diff --git a/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs b/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs index 3744007d9e45ac..bffbf999e7396f 100644 --- a/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs +++ b/src/tests/Regressions/coreclr/GitHub_62058/test62058.cs @@ -41,7 +41,7 @@ public static void CatchRethrow(Action action) { Console.Out.WriteLine("catch"); Console.Out.Flush(); - throw new Exception("catch", e); // throw; doesn't work either + throw new Exception("catch", e); } } From 62f2e7e1081ab4633a56c3869a1f82c3da08f849 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 20 Jan 2022 19:52:11 +0100 Subject: [PATCH 4/4] Reenable DllImportGenerator.Unit.Tests --- src/libraries/tests.proj | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj index d9391e0ec2c9d0..8d6a97b157d41a 100644 --- a/src/libraries/tests.proj +++ b/src/libraries/tests.proj @@ -334,11 +334,6 @@ Roslyn4.0.Tests.csproj" /> - - - - -