[webkit-reviews] review denied: [Bug 26838] Multithread support for JSC on UNIX : [Attachment 84113] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 26838: Multithread support for JSC on UNIX
https://bugs.webkit.org/show_bug.cgi?id=26838

Attachment 84113: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=84113&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84113&action=review

Looks good, but I think it could use another iteration. Eric, with regard to
your previous comment: Not having these stubs pretty much leaves Non-Safari
Pthread ports in a broken state. I think it's valid to separate the task of
filling the stubs and then fixing the style issues with the original file. I
think that Alex and I would be quite happy to tackle the cleanup here.

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:102
>> +static const int SIG_THREAD_SUSPEND_RESUME = SIGUSR2;
> 
> SIG_THREAD_SUSPEND_RESUME is incorrectly named. Don't use underscores in your
identifier names.  [readability/naming] [4]

I believe you should make this constant just follow WebKit coding style
conventions. g_threadSuspendResumeSignal or something similar.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:109
> +    sigset_t sigs;
> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIG_THREAD_SUSPEND_RESUME);
> +    sigsuspend(&sigs);

sigs -> signalSet.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:135
> +#if USE(PTHREADS)
> +	   struct sigaction act;
> +	   act.sa_handler = pthreadSignalHandlerSuspendResume;
> +	   sigemptyset(&act.sa_mask);
> +#ifdef SA_RESTART
> +	   act.sa_flags = SA_RESTART;
> +#else
> +	   act.sa_flags = 0;
> +#endif
> +	   sigaction(SIG_THREAD_SUSPEND_RESUME, &act, 0);
> +
> +	   sigset_t mask;
> +	   sigemptyset(&mask);
> +	   sigaddset(&mask, SIG_THREAD_SUSPEND_RESUME);
> +	   pthread_sigmask(SIG_UNBLOCK, &mask, 0);
> +#endif

I'm not sure if USE(PTHREADS) is appropriate ifdef here since it appears that
other platforms that already have an implementation for these stubs
USE(PTHREADS) as well. You probably want to add  !PLATFORM(DARWIN), etc to this
and abstract it into a #define at the top.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:368
> +#if HAVE(PTHREAD_NP_H) || PLATFORM(NETBSD)
> +    // e.g. on FreeBSD 5.4, neundorf at kde.org
> +    pthread_attr_get_np(platformThread, &regs);

I guess configure on NetBSD doesn't declare PTHREAD_NP_H? Is this bug
documented somewhere?

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:418
> +    int rc = pthread_attr_getstack(&regs, &stackBase, &stackSize);
> +    (void)rc; // FIXME: Deal with error code somehow? Seems fatal.

Is it really okay here to not check the return value of pthread_attr_getstack?
I'd rather see the FIXME resolved before this patch goes in.

> Source/JavaScriptCore/wtf/Platform.h:566
> +#if (ENABLE(WORKERS) || PLATFORM(GTK) || PLATFORM(IOS) || PLATFORM(MAC) ||
PLATFORM(WIN) || (PLATFORM(QT) && OS(DARWIN) && !ENABLE(SINGLE_THREADED))) &&
!defined(ENABLE_JSC_MULTIPLE_THREADS)

I don't feel like I know enough about this code to okay this change, but it
seems sane to me.