Bug 78389 - wtf/ThreadingWin.cpp doesn't build for 64-bit Windows
Summary: wtf/ThreadingWin.cpp doesn't build for 64-bit Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-10 14:21 PST by Kalev Lember
Modified: 2012-02-17 13:55 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.25 KB, patch)
2012-02-10 14:22 PST, Kalev Lember
eric: review-
Details | Formatted Diff | Diff
Patch v2 (51.42 KB, patch)
2012-02-15 03:09 PST, Kalev Lember
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (52.42 KB, patch)
2012-02-15 04:35 PST, Kalev Lember
no flags Details | Formatted Diff | Diff
Patch v4 (53.20 KB, patch)
2012-02-15 04:58 PST, Kalev Lember
aroben: review-
Details | Formatted Diff | Diff
Patch v5 (53.71 KB, patch)
2012-02-17 09:09 PST, Kalev Lember
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalev Lember 2012-02-10 14:21:03 PST
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]
Comment 1 Kalev Lember 2012-02-10 14:22:57 PST
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.
Comment 2 Simon Hausmann 2012-02-13 06:10:23 PST
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.
Comment 3 Kalev Lember 2012-02-13 06:25:01 PST
(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.
Comment 4 Adam Roben (:aroben) 2012-02-13 07:45:25 PST
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).
Comment 5 Eric Seidel (no email) 2012-02-13 11:42:21 PST
Comment on attachment 126582 [details]
Patch

The statements above sound like an r-.
Comment 6 Kalev Lember 2012-02-15 03:09:30 PST
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.
Comment 7 Early Warning System Bot 2012-02-15 03:48:09 PST
Comment on attachment 127149 [details]
Patch v2

Attachment 127149 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11525388
Comment 8 WebKit Review Bot 2012-02-15 04:15:28 PST
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.
Comment 9 Kalev Lember 2012-02-15 04:35:29 PST
Created attachment 127159 [details]
Patch v3

Oops, patch v2 had an infinite recursion in one of the ABI compat functions.
Comment 10 Kalev Lember 2012-02-15 04:58:01 PST
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?
Comment 11 Adam Roben (:aroben) 2012-02-17 08:25:15 PST
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.
Comment 12 Kalev Lember 2012-02-17 09:09:14 PST
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);
Comment 13 Adam Roben (:aroben) 2012-02-17 09:57:12 PST
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!
Comment 14 Kalev Lember 2012-02-17 10:22:24 PST
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.
Comment 15 WebKit Review Bot 2012-02-17 13:55:15 PST
Comment on attachment 127602 [details]
Patch v5

Clearing flags on attachment: 127602

Committed r108119: <http://trac.webkit.org/changeset/108119>
Comment 16 WebKit Review Bot 2012-02-17 13:55:21 PST
All reviewed patches have been landed.  Closing bug.