Bug 34922 – Mingw-w64 fixes for QtWebKit (WebCore and JavaScriptCore)

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Platform

Created attachment 48706 [details]
patch with fixes required by QtWebkit for mingw64

mingw-w64 can compile 64-bit binaries for Windows. While patching Qt 4.6, I came across some bugs (mostly pointer casts) that were problematic in the compilation of QtWebkit.

Patches are the same as merge request for Qt (but patch needed to be redirected here for Webkit component):
http://qt.gitorious.org/qt/qt/merge_requests/2308

[Benjamin Poulain]

Great you already posted the patches here :)
You need to create a Changelog for you patch.

Execute the script prepare-changelog:
WebKitTools/Script/prepare-changelog --bug 34922

Then, edit the changelog files to explain the changes, and submit the diff, changelog included.

Created attachment 48707 [details]
patch with fixes required by QtWebkit for mingw64

add missing changelogs to diff

[Zoltan Herczeg]

(In reply to comment #2)
> Created an attachment (id=48707) [details]
> patch with fixes required by QtWebkit for mingw64
> 
> add missing changelogs to diff

Hm, these stray defines are not the WebKit way. We have COMPILER(MINGW) and CPU(X86_64) in JavaScriptCore/wtf/Platform.h Their combination looks better... (but you still need to update these macro definitions for your platform)

Would also be interesting to see whether the JIT can be enabled on your platform (see ENABLE_JIT macros in the same header file).

Will fix this by editing platform.h, COMPILER(MINGW) section:

/* COMPILER(MINGW) - MinGW GCC */
/* COMPILER(MINGW64) - mingw-w64 GCC - only used as additional check to exclude mingw.org specific functions */
#ifdef __MINGW32__
#define WTF_COMPILER_MINGW 1
#include <_mingw.h> /* private MinGW header */
# ifdef __MINGW64_VERSION_MAJOR /* best way to check for mingw-w64 vs mingw.org */
# define WTF_COMPILER_MINGW64 1
# endif /* __MINGW64_VERSION_MAJOR */
#endif /* __MINGW32__ */

This is required to differentiate between the two
#if ( COMPILER(MINGW) && !COMPILER(MINGW64) )
but default to standard COMPILER(MINGW) behavior. Additional patched files will be added, but take a bit more time. I'm checking the build with
perl WebKitTools/Scripts/build-webkit --qt
from cmd.exe with mingw64/bin and bison, flex... in PATH.

[Zoltan Herczeg]

Ok, I understand your intention, but it is unusual in WebKit to concatenate the compiler and cpu id. We Have COMPILER(GCC), COMPILER(RVCT), CPU(SPARC64), CPU(PPC32), but we don't have COMPILER(MSVC64), or CPU(GCC_X86). This naming convention was not my idea, but we should follow them.

> #if ( COMPILER(MINGW) && !COMPILER(MINGW64) )

can't we simply use 

#if ( COMPILER(MINGW) && !CPU(X86_64) ) ?

The problem is, *if* mingw.org ever decides to bring out a 64-bit version, they still have access to different functions, and use a different crt. For example, in JavaScriptCore/runtime/Collector.cpp, mingw.org has its own implementation, whereas mingw-w64 allows the msvc codepath to be taken. Also, for the mingw-w32 variant (also from the mingw-w64 team), which produces 32-bit binaries, the given example will not compile, because some mingw.org functions (Collector.cpp:line 324:__mingw_aligned_free and line 233 __mingw_aligned_malloc) are not implemented in mingw-w64/w32, because they have the msvc functions. To see this difference, we either need a way to check __MINGW64_VERSION_MAJOR (the only stable way to check for the difference), which I implemented like so. I hope you understand the reasoning behind my decision.

PS: I did not concatenate the CPU in the COMPILER check, mingw-w32 will also register as COMPILER(MINGW64), because of the reason stated above.

Created attachment 48770 [details]
version 2 of patch, no changelog yet

Preliminary patch including new (additional) COMPILER(MINGW64) instead of ifdefs for __MINGW64_VERSION_MAJOR

Additional changes were made vs the previous patch (PluginView.cpp and TransformationMatrix.h), discovered when compiling webkit for Qt on mingw-w64.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/TextStream.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/text/TextStream.h:50:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/text/TextStream.h:50:  Extra space after (  [whitespace/parens] [2]
JavaScriptCore/wtf/Platform.h:954:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/plugins/win/PluginViewWin.cpp:1025:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/plugins/win/PluginViewWin.cpp:1025:  Extra space after (  [whitespace/parens] [2]
JavaScriptCore/runtime/Collector.cpp:569:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/runtime/Collector.cpp:569:  Extra space after (  [whitespace/parens] [2]
JavaScriptCore/runtime/Collector.cpp:579:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/runtime/Collector.cpp:579:  Extra space after (  [whitespace/parens] [2]
WebCore/platform/text/TextStream.cpp:112:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/text/TextStream.cpp:112:  Extra space after (  [whitespace/parens] [2]
Total errors found: 12


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

[Zoltan Herczeg]

(In reply to comment #6)

You mean mingw64 is more like a different platform instead of a 64 bit mingw compiler. Why didn't they give it a different name then? Anyway, would be interesting to hear the others opinion.

One more note: please remove the /home/Ruben/webkit-svn/WebKit/WebCore/bindings/scripts in WebKitTools/Scripts/build-webkit

Created attachment 48776 [details]
patch with fixes required by QtWebkit for mingw64 -v2

patch v2 proper with changelog modifications

Created attachment 48777 [details]
patch with fixes required by QtWebkit for mingw64 -v2

removed double changelog addition. Sorry about the mess.

Yes, in that aspect, mingw64 is a different platform, but it also uses much of mingw.org's functionality, and to keep backward compatibility with many projects (as a lot of projects, like ogg, vorbis, flac, gsl, fftw) build on mingw64 without change.

I hope I have elaborated enough to be convincing :)

[Gabor Loki]

>-#if COMPILER(MINGW)
>+#if ( COMPILER(MINGW) && !COMPILER(MINGW64) )

Although the style bot did not say anything about this change, I think this looks bad.
I have never seen such an #if construct. First of all, leave no space between
parentheses and its content! Additionally, in this case there is no reason to put
parentheses around this expression.

> /* COMPILER(MINGW) - MinGW GCC */
>-#if defined(MINGW) || defined(__MINGW32__)
>+/* COMPILER(MINGW64) - mingw-w64 GCC - only used as additional check to exclude mingw.org specific functions */
>+#if defined (__MINGW32__)
> #define WTF_COMPILER_MINGW 1
>-#endif
>+#include <_mingw.h> /* private MinGW header */
>+# if defined (__MINGW64_VERSION_MAJOR) /* best way to check for mingw-w64 vs mingw.org */
>+# define WTF_COMPILER_MINGW64 1
>+# endif /* __MINGW64_VERSION_MAJOR */
>+#endif /* __MINGW32__ */

Hmmm, __MINGW32__ is set for MINGW64 as well.
So, you do not need the following extra check on x86:

> #if PLATFORM(QT)
> #if (CPU(X86) && OS(WINDOWS) && COMPILER(MINGW) && GCC_VERSION >= 40100) \
>+    || (CPU(X86) && OS(WINDOWS) && COMPILER(MINGW64) && GCC_VERSION >= 40100) \

Created attachment 48865 [details]
patch with fixes required by QtWebkit for mingw64 -v2

Style fixes not caught by style-bot.

Created attachment 48905 [details]
patch with fixes required by QtWebkit for mingw64 -v3

 - Fixed broken patch that fixed style problems.
 - Changed getStackBase so that OS(WINDOWS) && CPU(X86) && COMPILER(GCC) uses the mingw.org asm block (and for obvious reasons not the msvc __asm one)

[Thierry Bastian]

*** Bug 35376 has been marked as a duplicate of this bug. ***

Any progress on this issue? Thanks.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Jocelyn Turcotte]

(In reply to comment #17)
> Any progress on this issue? Thanks.

I can't give you review+ but the patch looks good to me.

The only complaint I got is that COMPILER(MINGW64) might be confusing, especially if we start having stuff like COMPILER(MINGW64) && !CPU(X86_64) for MinGW-w32.

What do you think about adding the extra W: COMPILER(MINGWW64) ?

I understand it may be confusing, but the shorthand way of saying you're using the mingw-w64/w32 toolchain is mingw64. For example functions that are provided by the mingw64 vs mingw.org project are shared across bitness, so you need a bitness-independent way of checking the toolchain/compiler used).
The extra W would imply you are using "mingw-w64", but mingw64 could mean both *-w64 and *-w32 (IMHO).
I see it like this:
MINGW64 -> mingw-w64/w32
MINGWW64 -> mingw-w64
MINGWW32 -> mingw-w32 (necessary addition, which will forever be confused with regular MINGW)

I understand their project name isn't the best choice for stuff like this, but I presume it started as 64-bit only, but they then realized it could be much more and 32-bit as well.

[Adam Barth]

Comment on attachment 48905 [details]
patch with fixes required by QtWebkit for mingw64 -v3

+ #elif OS(WINDOWS) && CPU(X86_64) \

No need for a line break here.  (Several instances of this comment.)

- typedef unsigned long uword;
+ typedef uintptr_t uword;

Are you sure this change is correct?  It would seem to have effects beyond Mingw-w64.  Similarly, uint32 => uintptr_t.

Created attachment 51313 [details]
patch with fixes required by QtWebkit for mingw64 -v3

the line breaks have been removed.

firstly: uword is used here for pointer casts, which makes uintptr_t a good replacement
secondly: unsigned long in 32-bit in 32-bit and 64-bit windows, so that makes it a bad choice for this situation.

Created attachment 51327 [details]
patch with fixes required by QtWebkit for mingw64 -v3

now without hiccups (note to self, don't modify patch files by hand)

The new svn version has these patches applied, but in a bad way (my first attempt at a patch). This is why the latest patch fails to apply.

For example: http://svn.webkit.org/repository/webkit/trunk/JavaScriptCore/runtime/Collector.cpp line 323 has the !defined(__MINGW64_VERSION_MAJOR) appended to it. Anyone know what happened?

Found reference to this bug report in the changelog present in current svn:
https://bugs.webkit.org/show_bug.cgi?id=35607

Doesn't seem reviewed and stuff though??

[Benjamin Poulain]

(In reply to comment #25)
> Found reference to this bug report in the changelog present in current svn:
> https://bugs.webkit.org/show_bug.cgi?id=35607
> 
> Doesn't seem reviewed and stuff though??

From 35607, https://bug-35607-attachments.webkit.org/attachment.cgi?id=49852 already landed (see the history: https://bugs.webkit.org/show_activity.cgi?id=35607 ).

Well, it's a sucky patch IMHO. There is some stuff in there (like the "defined(__MINGW64_VERSION_MAJOR)") that goes straight against comment #3 here... among other things.

[Benjamin Poulain]

(In reply to comment #27)
> Well, it's a sucky patch IMHO. There is some stuff in there (like the
> "defined(__MINGW64_VERSION_MAJOR)") that goes straight against comment #3
> here... among other things.

I really can't help you, I don't know much about MinGW.
But you should definitely comment on https://bugs.webkit.org/show_bug.cgi?id=35607 

If you want, I can mark this task as a duplicate of 35607, and you can contribute directly on 35607 (it seems that one has gained more visibility).

If a patch of 35607 is not correct, you can add a patch in the task to fix it.

I'd like that. Will I have to make a patch that undoes the other patch or just patch over it? Thanks

[Benjamin Poulain]

(In reply to comment #29)
> I'd like that. Will I have to make a patch that undoes the other patch or just
> patch over it? Thanks

It is probably easier to patch over it.

*** This bug has been marked as a duplicate of bug 35607 ***