Bug 67864 – [Qt] Win32 builds with threads turned off

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

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.

[Andreas Kling]

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

[Jarred Nicholls]

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 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]

Created attachment 107004 [details]
Proposed DRT Patch

Ossy's suggestion (thanks, good call)

[Csaba Osztrogonác]

Comment on attachment 107004 [details]
Proposed DRT Patch

Clearing flags on attachment: 107004

Committed r94927: <http://trac.webkit.org/changeset/94927>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

> 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]

(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]

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]

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

[Geoffrey Garen]

> 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]

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]

Simon, what do you think about it?

[Jarred Nicholls]

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

[Csaba Osztrogonác]

(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]

(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]

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

[Jarred Nicholls]

(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 on attachment 107301 [details]
proposed fix

r=me

[Geoffrey Garen]

Seems like we have consensus.

[WebKit Review Bot]

Comment on attachment 107301 [details]
proposed fix

Clearing flags on attachment: 107301

Committed r95110: <http://trac.webkit.org/changeset/95110>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Paul]

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]

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

[Paul]

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

[Paul]

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]

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]

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]

(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]

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]

(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]

(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!