Summary: | [JSC] Get stack pointer without pthread attr | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, cdumez, cgarcia, clopez, cmarcelo, dbates, mark.lam, mcatanzaro, saam, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-04-22 09:00:25 PDT
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> |