Skip to content
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

Fix exception propagation over HW exception frame on macOS arm64 #63596

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
18 changes: 6 additions & 12 deletions src/coreclr/pal/src/exception/machexception.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,28 +369,22 @@ 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;

contextRecord->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;
pContext->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;
bool continueExecution;

{
// The exception object takes ownership of the exceptionRecord and contextRecord
PAL_SEHException exception(exceptionRecord, contextRecord);
PAL_SEHException exception(pExRecord, pContext, true);
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved

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;
*pContext = *exception.ExceptionPointers.ContextRecord;
*pExRecord = *exception.ExceptionPointers.ExceptionRecord;
}

// The exception records are destroyed by the PAL_SEHException destructor now.
Expand Down
16 changes: 8 additions & 8 deletions src/coreclr/pal/src/exception/seh-unwind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,9 @@ void GetContextPointers(unw_cursor_t *cursor, unw_context_t *unwContext, KNONVOL

Copy link
Member

@am11 am11 Jan 21, 2022

Choose a reason for hiding this comment

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

@janvorli, should this:

fpsimd_context* fp = GetNativeSigSimdContext(unwContext);
#define ASSIGN_FP_REG(fp, reg) if (fp) *(NEON128*) &fp->vregs[reg] = winContext->V[reg];
be changed to:

    fpsimd_context* fp = GetNativeSigSimdContext(unwContext);
#define ASSIGN_FP_REG(reg) if (fp) *(NEON128*) &fp->vregs[reg] = winContext->V[reg];

(only pass one argument to ASSIGN_FP_REG)
While working on #64043, I had a misconfiguration (related to UNWIND_CONTEXT_IS_UCONTEXT_T detection) and was getting errors during macro expansion, when this code compiled on linux arm64.

Copy link
Member

Choose a reason for hiding this comment

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

@am11 Seems to be the case considering the only use of ASSIGN_FP_REG has a single argument.

ASSIGN_FP_REG(8) \
ASSIGN_FP_REG(9) \
ASSIGN_FP_REG(10) \
ASSIGN_FP_REG(11) \
ASSIGN_FP_REG(12) \
ASSIGN_FP_REG(13) \
ASSIGN_FP_REG(14) \
ASSIGN_FP_REG(15) \
ASSIGN_FP_REG(16) \
ASSIGN_FP_REG(17) \
ASSIGN_FP_REG(18) \
ASSIGN_FP_REG(19) \
ASSIGN_FP_REG(20) \
ASSIGN_FP_REG(21) \
ASSIGN_FP_REG(22) \
ASSIGN_FP_REG(23) \
ASSIGN_FP_REG(24) \
ASSIGN_FP_REG(25) \
ASSIGN_FP_REG(26) \
ASSIGN_FP_REG(27) \
ASSIGN_FP_REG(28) \
ASSIGN_FP_REG(29) \
ASSIGN_FP_REG(30) \
ASSIGN_FP_REG(31)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this code was ever compiling and is dead? cl.exe on Windows and clang on Linux both fail the UNWIND_CONTEXT_IS_UCONTEXT_T test and set it to 0 on main. To unveil the problem, i.e. misconfiguration which I had -- in order to set it to 1, delete the last /include from:

list(INSERT CMAKE_REQUIRED_INCLUDES 0 ${CMAKE_CURRENT_SOURCE_DIR}/libunwind/include ${CMAKE_CURRENT_BINARY_DIR}/libunwind/include)

Copy link
Member Author

Choose a reason for hiding this comment

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

@am11 you are right, it is a dead code. I've added it when fixing floating point registers unwind because we had that code path for ARM64. However, the libunwind currently doesn't use the ucontext_t as unwind context for ARM64, IIRC a comment in there says it is done that way in order to reduce size of the unwind context. So everything for ARM64 with UNWIND_CONTEXT_IS_UCONTEXT_T set is not being ever compiled.
I think that we should just remove all of that and replace it with #error

#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)
{
Expand All @@ -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)
{
Expand Down
7 changes: 2 additions & 5 deletions src/coreclr/pal/src/exception/signal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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))
{
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/pal/src/include/pal/seh.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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_ */

60 changes: 60 additions & 0 deletions src/tests/Regressions/coreclr/GitHub_62058/test62058.cs
Original file line number Diff line number Diff line change
@@ -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);
}
}

public static void CatchIgnore(Action action)
{
try
{
action.Invoke();
}
catch (Exception)
{
Console.Out.WriteLine("ignore");
Console.Out.Flush();
}
}
}
9 changes: 9 additions & 0 deletions src/tests/Regressions/coreclr/GitHub_62058/test62058.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<ItemGroup>
<Compile Include="test62058.cs" />
</ItemGroup>
</Project>