Bug 171162 – [JSC] Get stack pointer without pthread attr

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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;
}

[Yusuke Suzuki]

Created attachment 307909 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 307910 [details]
Patch

[Yusuke Suzuki]

Created attachment 307911 [details]
Patch

[Yusuke Suzuki]

Created attachment 307912 [details]
Patch

[Mark Lam]

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.

[Mark Lam]

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;

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r215667: <http://trac.webkit.org/changeset/215667>