Some linux clients are encountering this assertion. Is there a way to either add alternative signal stack support? <rdar://problem/41765872>
See assertion in Thread::signalHandlerSuspendResume() in ThreadingPthreads.cpp.
Why alternative stack is not supported is, 1. when a signal handler with an alternative stack flag is invoked, it's stack pointer is an alternative stack. 2. while executig a signal handler, SIGUSR1 is emitted to suspend the thread 3. Nested signal handler is invoked, where machine context's stack pointer is an alternative stack. 4. stack scan is broken. I think this is not Linux specific. Even in Darwin, an alternative stack is not supported. 1. when a signal handler with an alternative stack flag is invoked, it's stack pointer is an alternative stack. 2. while executig a signal handler, thread_suspend is executed 3. saved thread context's stack pointer is pointing an alternative stack 4. stack scan is broken. In Linux, the way to avoid this issue is, 1. Do not use an alternative stack Due to the above reason, not using an alternative stack just fixes the problem. 2. Add SIGUSR1 to a signal mask when an alternative stack flag is specified. The root cause of the problem is that SIGUSR1 signal handler is invoked while executing a signal handler with an alternative stack. So setting SIGUSR1 to sa_mask can fix the issue. I think the current assertion is valid in both cases. In Darwin, we do not have the way to fix the issue except for not using an alternative stack. Mark, do you have any idea?
(In reply to Yusuke Suzuki from comment #2) > ... > I think this is not Linux specific. Even in Darwin, an alternative stack is > not supported. > > 1. when a signal handler with an alternative stack flag is invoked, it's > stack pointer is an alternative stack. > 2. while executig a signal handler, thread_suspend is executed > 3. saved thread context's stack pointer is pointing an alternative stack > 4. stack scan is broken. According to Thread::getRegisters(), OS(DARWIN) does not depend on the platform registers stashed away by Thread::signalHandlerSuspendResume(). Instead, it uses a thread_get_state() to get the registers of the target thread. I quite sure that thread_get_state() does not require the target thread to be the current thread. In fact, OS(DARWIN) suspends the target thread and reads its registers using thread_get_state(). Therefore, thread_get_state() has to be operating on a different thread there. Hence, this issue only applies to Linux.
(In reply to Mark Lam from comment #3) > According to Thread::getRegisters(), OS(DARWIN) does not depend on the > platform registers stashed away by Thread::signalHandlerSuspendResume(). > Instead, it uses a thread_get_state() to get the registers of the target > thread. I quite sure that thread_get_state() does not require the target > thread to be the current thread. In fact, OS(DARWIN) suspends the target > thread and reads its registers using thread_get_state(). Therefore, > thread_get_state() has to be operating on a different thread there. > > Hence, this issue only applies to Linux. What happens if 1. The target thread starts executing user-defined signal handlers with an alternative stack 2. While executing this handler, we suspend this thread by using thread_suspend. 3. Read the state by using thread_get_state() When reading the stack pointer at (3), is it pointing to an alternative stack?
(In reply to Yusuke Suzuki from comment #4) > (In reply to Mark Lam from comment #3) > > According to Thread::getRegisters(), OS(DARWIN) does not depend on the > > platform registers stashed away by Thread::signalHandlerSuspendResume(). > > Instead, it uses a thread_get_state() to get the registers of the target > > thread. I quite sure that thread_get_state() does not require the target > > thread to be the current thread. In fact, OS(DARWIN) suspends the target > > thread and reads its registers using thread_get_state(). Therefore, > > thread_get_state() has to be operating on a different thread there. > > > > Hence, this issue only applies to Linux. > > What happens if > > 1. The target thread starts executing user-defined signal handlers with an > alternative stack > 2. While executing this handler, we suspend this thread by using > thread_suspend. > 3. Read the state by using thread_get_state() > > When reading the stack pointer at (3), is it pointing to an alternative > stack? Good point. I don't think OS(DARWIN) uses an alternate signal stack, but I should double check.
Created attachment 344277 [details] Patch
Comment on attachment 344277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review > Source/WTF/wtf/ThreadingPthreads.cpp:371 > + Thread::yield(); Should we wait a bit here? idk.
Comment on attachment 344277 [details] Patch Attachment 344277 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8436661 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html
Created attachment 344287 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review r=me. Nice work and very clean. > Source/WTF/wtf/ThreadingPthreads.cpp:155 > + // 1. An user-defined signal handler is invoked with an alternative signal stack. /An user-defined/A user-defined/. > Source/WTF/wtf/ThreadingPthreads.cpp:158 > + // 4. A stack pointer saved in a machine context is pointing an alternative signal stack. I suggest "The stack pointer saved in the machine context will be pointing to the alternative signal stack." >> Source/WTF/wtf/ThreadingPthreads.cpp:371 >> + Thread::yield(); > > Should we wait a bit here? idk. I think this is fine because I see that we also use Thread::yield() elsewhere for purposes like this. Given that the probability of a suspension request coinciding with a user-defined signal handler is rare, I think we don't need to worry about any perf issues here. We already do a wait() in this loop which is way slower.
Comment on attachment 344277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review Thanks! >> Source/WTF/wtf/ThreadingPthreads.cpp:155 >> + // 1. An user-defined signal handler is invoked with an alternative signal stack. > > /An user-defined/A user-defined/. Fixed. >> Source/WTF/wtf/ThreadingPthreads.cpp:158 >> + // 4. A stack pointer saved in a machine context is pointing an alternative signal stack. > > I suggest "The stack pointer saved in the machine context will be pointing to the alternative signal stack." Fixed. >>> Source/WTF/wtf/ThreadingPthreads.cpp:371 >>> + Thread::yield(); >> >> Should we wait a bit here? idk. > > I think this is fine because I see that we also use Thread::yield() elsewhere for purposes like this. Given that the probability of a suspension request coinciding with a user-defined signal handler is rare, I think we don't need to worry about any perf issues here. We already do a wait() in this loop which is way slower. OK, sounds reasonable.
Committed r233613: <https://trac.webkit.org/changeset/233613>