wtf/ThreadingWin.cpp doesn't build for 64-bit Windows

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kalev Lember]

The GTK port build which recently switched from GThread to WTF threading, now fails with:
Source/JavaScriptCore/wtf/ThreadingWin.cpp:220:45: error: cast from 'void*' to 'unsigned int' loses precision [-fpermissive]

[Kalev Lember]

Created attachment 126582 [details]
Patch

The thread proc function passed to _beginthreadex() has unsigned int
return type. However, the actual worker function it calls returns
void*. The current code in wtfThreadEntryPoint() tried to cast the
void* to unsigned int, and then return that value. This however
doesn't work on 64-bit Windows where a pointer is a 64 bit and an int
is a 32 bit value.

Instead, just return 0 to signal success, and 1 for failure.

This shouldn't currently affect any client code, because
waitForThreadCompletion implementation in ThreadingWin doesn't
actually query the return value and does not return anything through
**result.

* wtf/ThreadingWin.cpp:
(WTF::wtfThreadEntryPoint): Avoid casting void* to unsigned int.

[Simon Hausmann]

Comment on attachment 126582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126582&action=review

> Source/JavaScriptCore/wtf/ThreadingWin.cpp:223
> -    return reinterpret_cast<unsigned>(result);
> +    if (!result)
> +        return 1;
> +
> +    return 0;

The main issue I see is that this breaks the waitForThreadCompletion API, which "promises" you a void* returned by the thread function, which will work on all platforms except on Windows (after your change).

I don't see anyone in WebKit actually relying on thread result, so perhaps the API should be changed at the same time as this fix, to ensure that nobody accidentally introduces code that relies on it and finds it working on their Mac/Linux but see it silently break on Windows.

[Kalev Lember]

(In reply to comment #2)
> The main issue I see is that this breaks the waitForThreadCompletion API, which "promises" you a void* returned by the thread function, which will work on all platforms except on Windows (after your change).

I believe my patch actually doesn't change the status quo: waitForThreadCompletion implementation in ThreadingWin.cpp as it is completely ignores the 'void** result' parameter. So with my change, it doesn't get any worse or any better -- it still won't return anything to the caller through '**result'.


Having said that, I agree that current situation where ThreadFunction returns a void* and then the thread's return value is completely ignored in waitForThreadCompletion on Windows isn't good. Should I try to come up with a patch to change the signature of ThreadFunction to return unsigned? It would be pretty invasive, touching a lot of files.

[Adam Roben (:aroben)]

Comment on attachment 126582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126582&action=review

>> Source/JavaScriptCore/wtf/ThreadingWin.cpp:223
>> +    return 0;
> 
> The main issue I see is that this breaks the waitForThreadCompletion API, which "promises" you a void* returned by the thread function, which will work on all platforms except on Windows (after your change).
> 
> I don't see anyone in WebKit actually relying on thread result, so perhaps the API should be changed at the same time as this fix, to ensure that nobody accidentally introduces code that relies on it and finds it working on their Mac/Linux but see it silently break on Windows.

I'd actually suggest changing the API first, then making any changes necessary for 64-bit on Windows separately. You have to be a little careful when changing the API, though, because Safari uses some of these WTF thread functions (see bug 25348 comment 12).

[Eric Seidel (no email)]

Comment on attachment 126582 [details]
Patch

The statements above sound like an r-.

[Kalev Lember]

Created attachment 127149 [details]
Patch v2

This patch implements the WTF threading API change and fixes up all client code. For Safari that uses the internal WTF threading API directly, I've also kept ABI-compatibility, so binaries should keep working without a rebuild. (It will still have to cope with the API change.)

Instead of changing the signature of ThreadFunction to return unsigned, I changed it to return void. The return value wasn't getting used anywhere, so by eliminating it, I could clean up the client code a bit.

[Early Warning System Bot]

Comment on attachment 127149 [details]
Patch v2

Attachment 127149 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11525388

[WebKit Review Bot]

Attachment 127149 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
	M	LayoutTests/fast/forms/radio-nested-labels.html
	M	LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kalev Lember]

Created attachment 127159 [details]
Patch v3

Oops, patch v2 had an infinite recursion in one of the ABI compat functions.

[Kalev Lember]

Created attachment 127165 [details]
Patch v4

There was another, a year-old createThread() compat function, which my patch apparently broke on MSVC. Fixed.

Is there any way to test patches on the build bots before submitting the patch on bugzilla?

[Adam Roben (:aroben)]

Comment on attachment 127165 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=127165&action=review

Nice! I think this is really close!

> Source/JavaScriptCore/wtf/Threading.cpp:95
> +typedef void* (*CompatFunction)(void* argument);

Maybe ThreadFunctionWithReturnValue would be a better name?

> Source/JavaScriptCore/wtf/Threading.cpp:99
> +struct CompatInvocation {

And this could be ThreadFunctionWithReturnValueInvocation.

> Source/JavaScriptCore/wtf/Threading.cpp:112
> +    OwnPtr<CompatInvocation> invocation = adoptPtr(static_cast<CompatInvocation*>(param));

I'd add a comment here:

// Balanced by .leakPtr() in createThread.

> Source/JavaScriptCore/wtf/Threading.cpp:123
> +    OwnPtr<CompatInvocation> invocation = adoptPtr(new CompatInvocation(entryPoint, data));
> +    ThreadIdentifier id = createThread(compatEntryPoint, invocation.get(), name);
> +
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +        return 0;

You can pass the result of .leakPtr() directly to createThread:

// Balanced by adoptPtr() in compatEntryPoint.
return createThread(compatEntryPoint, invocation.leakPtr(), name);

> Source/JavaScriptCore/wtf/Threading.cpp:147
> +    OwnPtr<CompatInvocation> invocation = adoptPtr(new CompatInvocation(entryPoint, data));
> +    ThreadIdentifier id = createThread(compatEntryPoint, invocation.get(), 0);
> +
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +        return 0;

Same advice here as above.

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:160
> +    OwnPtr<ThreadFunctionInvocation> invocation = adoptPtr(static_cast<ThreadFunctionInvocation*>(param));

I'd add a comment here:

// Balanced by .leakPtr() in createThreadInternal.

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:200
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +        return 0;

Our normal phrasing is "Balanced by adoptPtr() in wtfThreadEntryPoint."

This code is essentially checking whether the allocation of ThreadFunctionInvocation succeeded. This isn't something we typically do in WebKit. I'm not sure how to deal with the WARN_UNUSED_RESULT though. It looks like PingLoader.cpp does this:


// Leak the ping loader, since it will kill itself as soon as it receives a response.
PingLoader* leakedPingLoader = pingLoader.leakPtr();
UNUSED_PARAM(leakedPingLoader);

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:216
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +        return 0;

Same comments as above.

[Kalev Lember]

Created attachment 127602 [details]
Patch v5

Thank you for the review, Adam!

Applied all the suggested changes. The leakPtr() return value checking was only there to silence the compiler warnings, so I'm very glad there was a cleaner (or at least a more obvious) way to do it.

// Balanced by adoptPtr() in wtfThreadEntryPoint.
ThreadFunctionInvocation* leakedInvocation = invocation.leakPtr();
UNUSED_PARAM(leakedInvocation);

[Adam Roben (:aroben)]

Comment on attachment 127602 [details]
Patch v5

Have you tried running your local WebKit inside Safari to make sure the ABI compatibility is maintained?

Looks great!

[Kalev Lember]

Comment on attachment 127602 [details]
Patch v5

(In reply to comment #13)
> Have you tried running your local WebKit inside Safari to make sure the ABI compatibility is maintained?

No, I have not. Don't have a build environment for that; I'm working on webkitgtk here. What I have tested is webkitgtk with both pthreads and win32 threading.

The API change was really Simon's idea, I originally just wanted to get a 5-line build fix in.

[WebKit Review Bot]

Comment on attachment 127602 [details]
Patch v5

Clearing flags on attachment: 127602

Committed r108119: <http://trac.webkit.org/changeset/108119>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.