Bug 67864

Summary:

[Qt] Win32 builds with threads turned off

Product:

WebKit

Reporter:

Geoffrey Garen <ggaren>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, ggaren, harris.pc, hausmann, jarred, jturcotte, kling, ossy, webkit.review.bot, zoltan

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Windows XP

Attachments:

Description	Flags
Fix build errors	ossy: review+, ossy: commit-queue-
Proposed DRT Patch	none
proposed fix	none
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4	none
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4	none
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4	none

Geoffrey Garen

Reported 2011-09-09 14:20:33 PDT

As of http://trac.webkit.org/changeset/94872, the Qt Win32 build is broken: i486-mingw32-g++ -Wl,-enable-stdcall-fixup -Wl,-enable-auto-import -Wl,-enable-runtime-pseudo-reloc -Wl,-s -Wl,-subsystem,console -mthreads -o release/jsc.exe obj/release/jsc.o -L'./release' -L'/usr/local/Trolltech/Qt-4.7.3-mingw/lib' -ljscore -lwinmm -ladvapi32 -lQtCore4 ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x43): undefined reference to `__imp__pthread_self' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x88): undefined reference to `__imp__pthread_equal' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0xab): undefined reference to `__imp__pthread_equal' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x1f5): undefined reference to `__imp__pthread_key_delete' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x2d0): undefined reference to `__imp__pthread_key_create' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x338): undefined reference to `__imp__pthread_getspecific' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x366): undefined reference to `__imp__pthread_setspecific' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x36b): undefined reference to `__imp__pthread_self' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x386): undefined reference to `__imp__pthread_getw32threadhandle_np' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x588): undefined reference to `__imp__pthread_self' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x5cd): undefined reference to `__imp__pthread_equal' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x5f0): undefined reference to `__imp__pthread_equal' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0x9ba): undefined reference to `__imp__pthread_self' ./release/libjscore.a(MachineStackMarker.o):MachineStackMarker.cpp:(.text+0xa00): undefined reference to `__imp__pthread_equal' Here's a description of the problem: [9:12pm] ggaren: kling: MachineStackMarker.cpp is a little strange. Regardless of what a port does for other things (i.e., USE(PTHREAD) or not), MachineStackMarker.cpp assumes it can call the pthreads APIs for certain things like thread-specific data and access to self. [9:13pm] ggaren: kling: so, even though qt win32 doesn't use pthreads for most things, MachineStackMarker.cpp still expects it to link against the pthreads library. [9:13pm] ggaren: kling: and, it seems to #include all the pthreads headers, so the library seems to be present in the build; it's just not linked against. And my proposed solution: We'd like a Qt Win32 maintainer to get the Qt Win32 build to link against the pthreads library. I think this is possible because it already #includes the library's headers, so it must build with the library. If this is not possible, perhaps we can find another solution. I'm sorry, I'm just not familiar enough with the Qt Win32 port to fix this myself.

Attachments
Fix build errors (1.29 KB, patch) 2011-09-09 22:45 PDT, Jarred Nicholls	ossy: review+ ossy: commit-queue-	Details Formatted Diff Diff
Proposed DRT Patch (1.56 KB, patch) 2011-09-11 04:34 PDT, Jarred Nicholls	no flags	Details Formatted Diff Diff
proposed fix (1.99 KB, patch) 2011-09-14 00:23 PDT, Csaba Osztrogonác	no flags	Details Formatted Diff Diff
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4 (3.78 KB, patch) 2011-12-01 07:58 PST, Paul	no flags	Details Formatted Diff Diff
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4 (3.77 KB, patch) 2011-12-01 08:00 PST, Paul	no flags	Details Formatted Diff Diff
Patch to use pthreads on Windows, for the JSC included with QT 4.7.4 (3.80 KB, patch) 2011-12-01 17:17 PST, Paul	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Andreas Kling

Comment 1 2011-09-09 14:57:05 PDT

Adding some people. Also related: https://lists.webkit.org/pipermail/webkit-qt/2011-September/001874.html

Jarred Nicholls

Comment 2 2011-09-09 22:45:34 PDT

Created attachment 106966 [details] Fix build errors Pretty sure Qt wouldn't attempt multi-threads on anything other than unix (not even OS X). Attached is a patch that should fix the build. JSC is grossly pthread-dependent in the runtime and heap access in multi-thread-enabled blocks. A huge improvement would need to be made so platforms can provide their own means of threading, e.g., Qt can use their own PlatformThread (QThread). Based on what I'm seeing, the Win port uses pthread-win32 (http://sourceware.org/pthreads-win32/ and binaries here ftp://sourceware.org/pub/pthreads-win32/dll-latest/lib/) instead of the Win API threading functions in windows.h. Qt Win32 is likely never going to rely on that, so we'll need to keep Qt Win32 out of the loop for now and maybe create a bug to enhance that using Qt threading, or some other reasonable work around.

Csaba Osztrogonác

Comment 3 2011-09-11 00:13:52 PDT

Comment on attachment 106966 [details] Fix build errors r+, but please add comment to the platform.h to make developers not to try to remove it again until we find a proper way for supporting multithreading on Qt-Win platform.

Jarred Nicholls

Comment 4 2011-09-11 04:34:50 PDT

Created attachment 107004 [details] Proposed DRT Patch Ossy's suggestion (thanks, good call)

Csaba Osztrogonác

Comment 5 2011-09-11 11:33:05 PDT

Comment on attachment 107004 [details] Proposed DRT Patch Clearing flags on attachment: 107004 Committed r94927: <http://trac.webkit.org/changeset/94927>

Csaba Osztrogonác

Comment 6 2011-09-11 11:33:13 PDT

All reviewed patches have been landed. Closing bug.

Geoffrey Garen

Comment 7 2011-09-11 15:16:05 PDT

> r+, but please add comment to the platform.h to make developers > not to try to remove it again until we find a proper way for > supporting multithreading on Qt-Win platform. Guys, this is upside-down. We don't want to hold back JavaScriptCore development waiting for someone to figure out the threading story on QtWin32. This is especially true since, according to Andreas on IRC, nobody develops QtWin32, so we may be waiting forever. We want to remove ENABLE_JSC_MULTIPLE_THREADS and all related code entirely (along with ENABLE_WTF_MULTIPLE_THREADS and others), so development can proceed with the baseline assumption that all platforms at a minimum support the concept of a thread. This is a critical baseline abstraction for a number of important algorithms. (See my email to webkit-dev.) It would be nice if a QtWin32 developer would fix QtWin32 to support the baseline abstractions required by ENABLE_JSC_MULTIPLE_THREADS. But if that can't happen, the right solution isn't to hold JavaScriptCore at a standstill -- it's to remove QtWin32 from the core set of builders.

Jarred Nicholls

Comment 8 2011-09-11 16:01:46 PDT

(In reply to comment #7) > > r+, but please add comment to the platform.h to make developers > > not to try to remove it again until we find a proper way for > > supporting multithreading on Qt-Win platform. > > Guys, this is upside-down. We don't want to hold back JavaScriptCore development waiting for someone to figure out the threading story on QtWin32. This is especially true since, according to Andreas on IRC, nobody develops QtWin32, so we may be waiting forever. If necessary, I'll work on it. I have products that need Qt-Win32 support so breaking it perpetually is not going to be something I support. I can keep a patch around to put the status quo back into place until I fix it all up. > > We want to remove ENABLE_JSC_MULTIPLE_THREADS and all related code entirely (along with ENABLE_WTF_MULTIPLE_THREADS and others), so development can proceed with the baseline assumption that all platforms at a minimum support the concept of a thread. This is a critical baseline abstraction for a number of important algorithms. (See my email to webkit-dev.) Before going further, wouldn't it be wise to then abstract the notion of a thread rather than going against pthread as if it's the only option, before proceeding with development? I haven't analyzed this more than 10 minutes to know if that's even allowable without breaking the current interface, i.e., changing pthread specific signatures to library independent ones that support a client interface et al. > > It would be nice if a QtWin32 developer would fix QtWin32 to support the baseline abstractions required by ENABLE_JSC_MULTIPLE_THREADS. But if that can't happen, the right solution isn't to hold JavaScriptCore at a standstill -- it's to remove QtWin32 from the core set of builders. I wouldn't call that a solution to the problem, since the actual problem is JSC being married to pthread in the multi-threaded areas. But it is what needs to happen if we have to remove those flags while the real solution is being implemented. So, since there's no set timeline on getting JSC pthread-independent, let's either reopen this ticket or create a new one to do so, and keep Qt-Win32 out of the core build set. When will you be removing these flags for good Geoff? Are you ready to do it immediately after removing QtWin from core builders, or are you still doing other cleanups first?

Csaba Osztrogonác

Comment 9 2011-09-12 02:32:18 PDT

I agree that you want to remove ENABLE_JSC_MULTIPLE_THREADS, I have no objection, I don't want to block the development of JSC. But you landed this patch at 23:42 (in Europe timezone) on friday. I think it isn't the best time for QtWebKit developers to find a proper way to fix the Windows build. We should give Jarred a chance (1-2 days) to fix it on weekdays.

Csaba Osztrogonác

Comment 10 2011-09-12 02:33:06 PDT

Reopen, because the landed patch was only a workaround to make Qt win build work until the proper fix.

Geoffrey Garen

Comment 11 2011-09-12 18:00:12 PDT

> If necessary, I'll work on it. Thanks, Jarred. I think it is necessary at this point; no on else has spoken up as a QtWin32 maintainer. As I said above, I'd recommend starting with trying to get QtWin32 to link against libpthread, as other Win platforms do. > Before going further, wouldn't it be wise to then abstract the notion of a thread rather than going > against pthread as if it's the only option, before proceeding with development? New thread-related code uses the WTF thread abstractions, so designing a thread abstraction is not a prerequisite to core VM development. The thing that's breaking QtWin32 right now is turning on old thread-related code, since that code was written before the WTF thread abstractions existed. We need to turn on the old code to establish the baseline assumption that "threads exist". I believe that you could fix QtWin32 by porting MachineStackMarker.cpp to WTF thread abstractions like ThreadSpecific<T>. (But I still think, in the short term, that linking against libpthread is the path of least resistance.) > When will you be removing these flags for good Geoff? Are you ready to do it immediately after > removing QtWin from core builders, or are you still doing other cleanups first? I'm ready with a patch now, but I'm willing to wait if you're actively working on this.

Csaba Osztrogonác

Comment 12 2011-09-14 00:23:16 PDT

Created attachment 107301 [details] proposed fix In my opinion QtWebKit on Windows should depends on pthread library. Unfortunately it isn't packed with QtSDK, so user have to install it manually from here: http://sourceware.org/pthreads-win32/ I tested it, and it works fine with MinGW, MSVC and on our bot (cross-MinGW on Linux) If this patch lands I'll update http://trac.webkit.org/wiki/BuildingQtOnWindows to help developers how can they install pthread.

Csaba Osztrogonác

Comment 13 2011-09-14 00:28:53 PDT

Simon, what do you think about it?

Jarred Nicholls

Comment 14 2011-09-14 00:32:35 PDT

This seems like blasphemy, but let's do it. If Apple can do it, we can do it ;)

Csaba Osztrogonác

Comment 15 2011-09-14 03:10:43 PDT

(In reply to comment #14) > This seems like blasphemy, but let's do it. If Apple can do it, we can do it ;) Why is it blasphemy? :o Until QtWebKit can't use native Windows threading, we should use pthread not to block the development of JSC. When you or someone else make it possible to use win threading we can remove pthread dependency.

Jocelyn Turcotte

Comment 16 2011-09-14 03:55:16 PDT

(In reply to comment #15) > Why is it blasphemy? :o Until QtWebKit can't use native Windows threading, we should use pthread not to block the development of JSC. When you or someone else make it possible to use win threading we can remove pthread dependency. Qt usually ship all dependencies with it's modules, so it means that we have to have a copy of libpthread in Qt, in WebKit, or switch to win threading before the next release to support Windows. I think it's alright at least for now to depend on libpthread, it would be nice to also update the Windows build wiki page if we add a dependency now. We can change it back if we remove it. But we have to agree on a final solution before we ship.

Jocelyn Turcotte

Comment 17 2011-09-14 04:17:47 PDT

(In reply to comment #16) Sorry, didn't see you said would update the wiki on comment 12 :)

Jarred Nicholls

Comment 18 2011-09-14 09:53:18 PDT

(In reply to comment #15) > (In reply to comment #14) > > This seems like blasphemy, but let's do it. If Apple can do it, we can do it ;) > > Why is it blasphemy? :o Until QtWebKit can't use native Windows threading, we should use pthread not to block the development of JSC. When you or someone else make it possible to use win threading we can remove pthread dependency. Yeah I didn't mean it like that. Bringing in a library with its last release in 2006 is good times :) Optimally JSC would go straight to Win API or via WTF interface - we are all on the same page.

Geoffrey Garen

Comment 19 2011-09-14 10:55:48 PDT

Comment on attachment 107301 [details] proposed fix r=me

Geoffrey Garen

Comment 20 2011-09-14 10:56:09 PDT

Seems like we have consensus.

WebKit Review Bot

Comment 21 2011-09-14 12:09:11 PDT

Comment on attachment 107301 [details] proposed fix Clearing flags on attachment: 107301 Committed r95110: <http://trac.webkit.org/changeset/95110>

WebKit Review Bot

Comment 22 2011-09-14 12:09:17 PDT

All reviewed patches have been landed. Closing bug.

Paul

Comment 23 2011-12-01 07:53:49 PST

I am using QT 4.7.4 with the included JavascriptCore. I'm compiling for Win32 with MSVC 2008. Running on Win7-64. I'm using QScriptEngine (and therefore, JavascriptCore) in multiple threads, and while that seems to work fine (even before this patch), every time the app closed, it would crash on destruction. The attached patch did not resolve this issue, so I investigated further. This patch does not completely enable pthreads properly for me, so you end up with pthreads partially enabled, but JSC will still continue to use QMutex and other QT-related threading classes. I fixed this problem, see attached patch. I am aware that the development has moved on somewhat since this bug was closed, eg https://bugs.webkit.org/show_bug.cgi?id=72155 But I am not sure if anyone is seeing the same problems I am on Windows. I expect most developers are on Macs/Linux (where I didn't have any issues). I have attached a patch for your consideration. It works against QT 4.7.4 so it will need adjustments to apply to the trunk. Can we reopen this bug? Thanks, Paul

Paul

Comment 24 2011-12-01 07:56:48 PST

by the way, the patch depends on pthread-win32.

Paul

Comment 25 2011-12-01 07:58:01 PST

Created attachment 117422 [details] Patch to use pthreads on Windows, for the JSC included with QT 4.7.4

Paul

Comment 26 2011-12-01 08:00:45 PST

Created attachment 117423 [details] Patch to use pthreads on Windows, for the JSC included with QT 4.7.4 required slight correction

WebKit Review Bot

Comment 27 2011-12-01 08:03:19 PST

Attachment 117423 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Csaba Osztrogonác

Comment 28 2011-12-01 08:08:47 PST

Qt 4.7.4 contains QtWebKit-2.0 http://trac.webkit.org/wiki/QtWebKitRelease20, and using trunk WebKit with Qt 4.7.4 isn't supported officially. But feel free to build your own Qt 4.7.4 with trunk WebKit with your patch. :)

Paul

Comment 29 2011-12-01 08:15:57 PST

(In reply to comment #28) > Qt 4.7.4 contains QtWebKit-2.0 http://trac.webkit.org/wiki/QtWebKitRelease20, and using trunk WebKit with Qt 4.7.4 isn't supported officially. But feel free to build your own Qt 4.7.4 with trunk WebKit with your patch. :) Thanks for the reply, However, I am not using trunk WebKit with Qt 4.7.4. I'm just patching what came with it. I am submitting the patch, so that hopefully this threading issue can be cleared up properly in the future. I don't know what the status of Trunk's threading is, but if its broken like QtWebKitRelease20 then maybe my patch could help. Best Regards, Paul

Paul

Comment 30 2011-12-01 17:17:07 PST

Created attachment 117529 [details] Patch to use pthreads on Windows, for the JSC included with QT 4.7.4 Fix to ensure QMake links to the correct pthreadVC2 dll (debug/release).

Simon Hausmann

Comment 31 2011-12-02 00:31:38 PST

(In reply to comment #29) > (In reply to comment #28) > > Qt 4.7.4 contains QtWebKit-2.0 http://trac.webkit.org/wiki/QtWebKitRelease20, and using trunk WebKit with Qt 4.7.4 isn't supported officially. But feel free to build your own Qt 4.7.4 with trunk WebKit with your patch. :) > > Thanks for the reply, > > However, I am not using trunk WebKit with Qt 4.7.4. > I'm just patching what came with it. > > I am submitting the patch, so that hopefully this threading issue can be cleared up properly in the future. I don't know what the status of Trunk's threading is, but if its broken like QtWebKitRelease20 then maybe my patch could help. The situation is indeed fixed in trunk. JSC_MULTIPLE_THREADS guards have been removed and instead it is considered enabled by default now. Also the Qt port in trunk now uses ThreadingWin.cpp on Windows (including a dependency to pthreads-win).

Paul

Comment 32 2011-12-02 15:47:16 PST

(In reply to comment #31) > (In reply to comment #29) > > (In reply to comment #28) > > > Qt 4.7.4 contains QtWebKit-2.0 http://trac.webkit.org/wiki/QtWebKitRelease20, and using trunk WebKit with Qt 4.7.4 isn't supported officially. But feel free to build your own Qt 4.7.4 with trunk WebKit with your patch. :) > > > > Thanks for the reply, > > > > However, I am not using trunk WebKit with Qt 4.7.4. > > I'm just patching what came with it. > > > > I am submitting the patch, so that hopefully this threading issue can be cleared up properly in the future. I don't know what the status of Trunk's threading is, but if its broken like QtWebKitRelease20 then maybe my patch could help. > > The situation is indeed fixed in trunk. JSC_MULTIPLE_THREADS guards have been removed and instead it is considered enabled by default now. Also the Qt port in trunk now uses ThreadingWin.cpp on Windows (including a dependency to pthreads-win). Ok great :) Thanks!

Note You need to log in before you can comment on or make changes to this bug.