-
-
Notifications
You must be signed in to change notification settings - Fork 192
fix: AOT interop with managed .NET runtimes #1392
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
base: master
Are you sure you want to change the base?
Changes from all commits
c1bd96a
45dccd6
02ebf36
81293d8
10ba6bd
d9eafcd
1bf0f52
060b18b
d4dd399
4da4c4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,6 +455,57 @@ registers_from_uctx(const sentry_ucontext_t *uctx) | |
return registers; | ||
} | ||
|
||
#ifdef SENTRY_PLATFORM_LINUX | ||
static uintptr_t | ||
get_stack_pointer(const sentry_ucontext_t *uctx) | ||
{ | ||
# if defined(__i386__) | ||
return uctx->user_context->uc_mcontext.gregs[REG_ESP]; | ||
# elif defined(__x86_64__) | ||
return uctx->user_context->uc_mcontext.gregs[REG_RSP]; | ||
# elif defined(__arm__) | ||
return uctx->user_context->uc_mcontext.arm_sp; | ||
# elif defined(__aarch64__) | ||
return uctx->user_context->uc_mcontext.sp; | ||
# elif defined(__mips__) || defined(__mips64__) | ||
return uctx->user_context->uc_mcontext.gregs[29]; // REG_SP | ||
# elif defined(__riscv) | ||
return uctx->user_context->uc_mcontext.__gregs[2]; // REG_SP | ||
# elif defined(__s390x__) | ||
return uctx->user_context->uc_mcontext.gregs[15]; | ||
# else | ||
SENTRY_WARN("get_stack_pointer is not implemented for this architecture. " | ||
"Signal chaining may not work as expected."); | ||
return NULL; | ||
# endif | ||
} | ||
|
||
static uintptr_t | ||
get_instruction_pointer(const sentry_ucontext_t *uctx) | ||
{ | ||
# if defined(__i386__) | ||
return uctx->user_context->uc_mcontext.gregs[REG_EIP]; | ||
# elif defined(__x86_64__) | ||
return uctx->user_context->uc_mcontext.gregs[REG_RIP]; | ||
# elif defined(__arm__) | ||
return uctx->user_context->uc_mcontext.arm_pc; | ||
# elif defined(__aarch64__) | ||
return uctx->user_context->uc_mcontext.pc; | ||
# elif defined(__mips__) || defined(__mips64__) | ||
return uctx->user_context->uc_mcontext.pc; | ||
# elif defined(__riscv) | ||
return uctx->user_context->uc_mcontext.__gregs[0]; // REG_PC | ||
# elif defined(__s390x__) | ||
return uctx->user_context->uc_mcontext.psw.addr; | ||
# else | ||
SENTRY_WARN( | ||
"get_instruction_pointer is not implemented for this architecture. " | ||
"Signal chaining may not work as expected."); | ||
return NULL; | ||
# endif | ||
} | ||
#endif | ||
|
||
static sentry_value_t | ||
make_signal_event( | ||
const struct signal_slot *sig_slot, const sentry_ucontext_t *uctx) | ||
|
@@ -533,20 +584,6 @@ handle_ucontext(const sentry_ucontext_t *uctx) | |
|
||
SENTRY_INFO("entering signal handler"); | ||
|
||
const struct signal_slot *sig_slot = NULL; | ||
for (int i = 0; i < SIGNAL_COUNT; ++i) { | ||
#ifdef SENTRY_PLATFORM_UNIX | ||
if (SIGNAL_DEFINITIONS[i].signum == uctx->signum) { | ||
#elif defined SENTRY_PLATFORM_WINDOWS | ||
if (SIGNAL_DEFINITIONS[i].signum | ||
== uctx->exception_ptrs.ExceptionRecord->ExceptionCode) { | ||
#else | ||
# error Unsupported platform | ||
#endif | ||
sig_slot = &SIGNAL_DEFINITIONS[i]; | ||
} | ||
} | ||
|
||
#ifdef SENTRY_PLATFORM_UNIX | ||
// inform the sentry_sync system that we're in a signal handler. This will | ||
// make mutexes spin on a spinlock instead as it's no longer safe to use a | ||
|
@@ -568,19 +605,54 @@ handle_ucontext(const sentry_ucontext_t *uctx) | |
// handler and that would mean we couldn't enter this handler with | ||
// the next signal coming in if we didn't "leave" here. | ||
sentry__leave_signal_handler(); | ||
if (!options->enable_logging_when_crashed) { | ||
sentry__logger_enable(); | ||
} | ||
|
||
uintptr_t ip = get_instruction_pointer(uctx); | ||
uintptr_t sp = get_stack_pointer(uctx); | ||
|
||
// invoke the previous handler (typically the CLR/Mono | ||
// signal-to-managed-exception handler) | ||
invoke_signal_handler( | ||
uctx->signum, uctx->siginfo, (void *)uctx->user_context); | ||
|
||
// If the execution returns here in AOT mode, and the instruction | ||
// or stack pointer were changed, it means CLR/Mono converted the | ||
// signal into a managed exception and transferred execution to a | ||
// managed exception handler. | ||
// https://github.com/dotnet/runtime/blob/6d96e28597e7da0d790d495ba834cc4908e442cd/src/mono/mono/mini/exceptions-arm64.c#L538 | ||
if (ip != get_instruction_pointer(uctx) | ||
|| sp != get_stack_pointer(uctx)) { | ||
SENTRY_DEBUG("runtime converted the signal to a managed " | ||
"exception, we do not handle the signal"); | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Signal Chaining Logic Fails on Some ArchitecturesThe new signal chaining logic has two issues: on architectures where instruction and stack pointers aren't retrieved, it incorrectly proceeds with crash handling even after a runtime converts a signal. Additionally, when a signal conversion is detected and the function returns early, the internal signal handler state becomes unbalanced. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is absolutely correct, but the only side-effect currently visible is for the logging toggle. Similarly, to how we "leave" the signal handler before chaining, we must also re-enable logging immediately after "leaving" and disable it again before re-entering, because if it were a managed code exception, we want logging to remain enabled. We can also move the entire However, I think both have a lower priority than figuring out the signaling sequence of both runtimes and how we can align them. |
||
|
||
// let's re-enter because it means this was an actual native crash | ||
if (!options->enable_logging_when_crashed) { | ||
sentry__logger_disable(); | ||
} | ||
sentry__enter_signal_handler(); | ||
SENTRY_DEBUG( | ||
"return from runtime signal handler, we handle the signal"); | ||
} | ||
#endif | ||
|
||
const struct signal_slot *sig_slot = NULL; | ||
for (int i = 0; i < SIGNAL_COUNT; ++i) { | ||
#ifdef SENTRY_PLATFORM_UNIX | ||
if (SIGNAL_DEFINITIONS[i].signum == uctx->signum) { | ||
#elif defined SENTRY_PLATFORM_WINDOWS | ||
if (SIGNAL_DEFINITIONS[i].signum | ||
== uctx->exception_ptrs.ExceptionRecord->ExceptionCode) { | ||
#else | ||
# error Unsupported platform | ||
#endif | ||
sig_slot = &SIGNAL_DEFINITIONS[i]; | ||
} | ||
} | ||
|
||
#ifdef SENTRY_PLATFORM_UNIX | ||
// use a signal-safe allocator before we tear down. | ||
sentry__page_allocator_enable(); | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -51,13 +51,16 @@ static void Main(string[] args) | |||||||||||||
{ | ||||||||||||||
Console.WriteLine("dereference a NULL object from managed code"); | ||||||||||||||
var s = default(string); | ||||||||||||||
var c = s.Length; | ||||||||||||||
var c = s!.Length; | ||||||||||||||
} | ||||||||||||||
catch (NullReferenceException exception) | ||||||||||||||
catch (NullReferenceException) | ||||||||||||||
{ | ||||||||||||||
Console.WriteLine("dereference another NULL object from managed code"); | ||||||||||||||
var s = default(string); | ||||||||||||||
var c = s.Length; | ||||||||||||||
if (args is ["managed-exception"]) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, the test re-triggers a sentry-native/tests/fixtures/dotnet_signal/Program.cs Lines 56 to 61 in 695f4a4
I put it behind the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, this highlights a similar divergence to the one I previously raised with the test assertion. Because the initial reason for doing this was to have two managed exceptions, where one is caught in managed code and the other isn't, both would end up in our signal handler. Neither should create a native event, at least that was my assumption, since they both have nothing to do with an actual native crash, because you would get a stack trace of the runtime (or typically much less than that because whichever stackwalker is in effect will not have sufficient information to walk the runtime's stack). In the AOT case, though, and apparently Mono too, since CLR AOT seems to be based on Mono AOT, you not only observe two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify: I think it is acceptable for Mono to raise a It is okay, because Sentry does install a top-level handler for .NET anyway, right? So the chance of triggering the But instead of preventing it from happening, I would let the C# code run as initially intended and then show inside the test that the serialized envelope exists in the AOT/Mono case where two managed exceptions are raised, but that it is a SIGABRT and not the SIGSEGV that triggered the managed code exception. This way, you don't hide that behavior and explicitly show the difference between the two implementations in the test assertions. Does that make sense? I think developing a heuristic for ignoring that particular |
||||||||||||||
{ | ||||||||||||||
Console.WriteLine("dereference another NULL object from managed code"); | ||||||||||||||
var s = default(string); | ||||||||||||||
var c = s!.Length; | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
is
#error
or a runtime warning a better choice here? i ended up with a warning because i thought it would be annoying to potentially break builds for irrelevant platforms that Sentry .NET doesn't even supportThere 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.
It is fine either way, but I would also err on giving a runtime response rather than failing at build time, as the entire execution path is optional.