pthread_ attr operations are not safe for calling it when the thread is suspended since it can invoke malloc / free. We would like to retrieve stack pointer without using pthread attr operations, like, NEVER_INLINES void* getPossibleStackPointer() { volatile void* stackLocation = nullptr; return &stackLocation; }
Created attachment 307909 [details] Patch
Comment on attachment 307909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307909&action=review > Source/WTF/wtf/ThreadingPthreads.cpp:101 > +} Now, we do not consider about alternative signal stack explicitly. Originally, it is not safe with conservative GC by nature.
Created attachment 307910 [details] Patch
Created attachment 307911 [details] Patch
Created attachment 307912 [details] Patch
Comment on attachment 307912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307912&action=review r=me with fixes. > Source/WTF/wtf/ThreadingPthreads.cpp:115 > + int ret = sigaltstack(nullptr, &stack); typo: /sigaltstack/signaltstack/ > Source/WTF/wtf/ThreadingPthreads.cpp:136 > + ASSERT_WITH_MESSAGE(!isOnAlternativeSignalStack(), "Do not support alternative signal stack. Consider disabling concurrent GC."); Let's change the message to "Using an alternative signal stack is not supported. Consider disabling the concurrent GC." Also, I suggest making this a RELEASE_ASSERT. I think this code is not performance critical, and this failure is critical and may be hard to detect if not asserted.
Comment on attachment 307912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307912&action=review >> Source/WTF/wtf/ThreadingPthreads.cpp:115 >> + int ret = sigaltstack(nullptr, &stack); > > typo: /sigaltstack/signaltstack/ Yusuke pointed out that I was wrong. sigaltstack is correct. > Source/WTF/wtf/ThreadingPthreads.cpp:117 > + ASSERT_UNUSED(ret, ret); > + return stack.ss_flags == SS_ONSTACK; If this assert is critical, you can combine it with the return value instead: return ret && stack.ss_flags == SS_ONSTACK;
Comment on attachment 307912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307912&action=review Thanks! > Source/WTF/wtf/ThreadingPthreads.cpp:116 > + ASSERT_UNUSED(ret, ret); This will be changed to RELEASE_ASSERT(!ret). >> Source/WTF/wtf/ThreadingPthreads.cpp:117 >> + return stack.ss_flags == SS_ONSTACK; > > If this assert is critical, you can combine it with the return value instead: > > return ret && stack.ss_flags == SS_ONSTACK; I think this |ret| never becomes non-zero. So, if it is non-zero, some crazy thing happens I think. I would like to have two assertions for that to investigate what is wrong if the assertion is fired. >> Source/WTF/wtf/ThreadingPthreads.cpp:136 >> + ASSERT_WITH_MESSAGE(!isOnAlternativeSignalStack(), "Do not support alternative signal stack. Consider disabling concurrent GC."); > > Let's change the message to "Using an alternative signal stack is not supported. Consider disabling the concurrent GC." > > Also, I suggest making this a RELEASE_ASSERT. I think this code is not performance critical, and this failure is critical and may be hard to detect if not asserted. Yeah, changed.
Comment on attachment 307912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307912&action=review >>> Source/WTF/wtf/ThreadingPthreads.cpp:136 >>> + ASSERT_WITH_MESSAGE(!isOnAlternativeSignalStack(), "Do not support alternative signal stack. Consider disabling concurrent GC."); >> >> Let's change the message to "Using an alternative signal stack is not supported. Consider disabling the concurrent GC." >> >> Also, I suggest making this a RELEASE_ASSERT. I think this code is not performance critical, and this failure is critical and may be hard to detect if not asserted. > > Yeah, changed. I've considered about it a bit more. This signal function is guarded by a global mutex, and GC / sampling profiler invokes it repeatedly. So, I think this function is a bit frequently called. And sigaltstack is system call. I think it is not good. Temporary, I would like to make it usual ASSERT for this reason.
Committed r215667: <http://trac.webkit.org/changeset/215667>