Fix ThreadAbort on Unix and reenable the related tests #73399
Conversation
|
I recommend reviewing with whitespace comparison disabled, I've again changed indentation of a large block of a code that makes the change look much larger (https://github.com/dotnet/runtime/pull/73399/files?w=1) |
When the HandleSuspensionForInterruptedThread invokes Thread::HandleThreadAbort and the thread abort exception is thrown from there, runtime fails with unhandled C++ exception because there was a missing guard that catches the PAL_SEHException and invokes DispatchManagedException to propagate it further through managed code. This change fixes that and also addresses an issue with activation injection in Thread::UserAbort in the case of infinite timeout (it would not inject the activation in that case).
janvorli
force-pushed
the
fix-unix-threadabort
branch
from
688d6c4
to
bcc622d
Compare
| UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; | ||
| UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; | ||
|
|
||
| frame.Pop(pThread); |
There was a problem hiding this comment.
Should we pop the frame right after PulseGCMode() ?
Is the frame used by the thread abort throw?
There was a problem hiding this comment.
Yes, it is used by the thread abort throw, that's why it is at this place.
| pThread->UnmarkRedirectContextInUse(m_Regs); | ||
| m_Regs = NULL; | ||
| if (pThread->UnmarkRedirectContextInUse(m_Regs)) | ||
| { |
There was a problem hiding this comment.
Here - can we just check UseContextBasedThreadRedirection() and if not, then not call Unmark and not assign null?
| } | ||
| void NeedStackCrawl() | ||
| { | ||
| m_pThread->SetThreadState(Thread::TS_StackCrawlNeeded); |
There was a problem hiding this comment.
Nit: This assumes NeedStackCrawl is never called on a non-Activated instance. We might want to add an assert or a check here.
|
There is a problem on Alpine Linux, all three architectures crash in the thread abort tests. I am investigating it. |
On Alpine Linux, libunwind cannot unwind correctly over signal frames. We have been handling that for hardware exceptions, but with the new possibility of thread abort exception being thrown from the activation handler, we need to apply the same treatment to the activation signal too. We save a location of the windows style context in the activation_injection_handler and a return address in this function for a call to the actual registered handler invocation. Then in PAL_VirtualUnwind, when we detect that we are trying to unwind from this location, we use the saved context instead of calling libunwind.
|
Fixed the issue |
|
The CI failure is #73247 |
ghost
locked as resolved and limited conversation to collaborators
When the HandleSuspensionForInterruptedThread invokes Thread::HandleThreadAbort
and the thread abort exception is thrown from there, runtime fails with
unhandled C++ exception because there was a missing guard that catches the
PAL_SEHException and invokes DispatchManagedException to propagate it further
through managed code.
This change fixes that and also addresses an issue with activation injection
in Thread::UserAbort in the case of infinite timeout (it would not inject the
activation in that case).
It also reenables the controlled execution tests on Unix.
I have tested it on both Unix and CET enabled Windows.