Bug 133028 – [GTK][META] Build break of 2.4.x under mingw32/msys

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Milan Crha]

I'm trying to build webkitgtk under mingw32/msys, which fails. I managed to cook some fixes, thus apart of already filled bug #132814, bug #132816 and bug #132856, I'd like to offer my changes for inclusion. I do not know what you prefer, whether a single patch, or rather split, but as the canges aren't related, then I'll split it, but if you prefer, I can join it into one too.

[Milan Crha]

Created attachment 231627 [details]
ource/autotools/FindDependencies.m4 changes

a) use icu-i18n pkg-config on mingw, if available (they may know better what to link to and where the files are - I noticed the not ein the file above this change, but on mingw it's fine, using icu4c-53_1-src.tgz)

b) on windows/mingw32, the OpenGL library is opengl32, not GL

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Milan Crha]

Created attachment 231628 [details]
preprocessor.pm/make_names.pl changes

a) do not hardcode gcc path for msys
b) use cygwin code in applyPreprocessor for msys as well

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Milan Crha]

Created attachment 231630 [details]
Source/ThirdParty/ANGLE changes

The ossource_posix.cpp doesn't work for windows, thus build it conditionally and add an ossource_win.cpp to the webkit sources. This file is taken unchanged from the ANGLE project (which might be a reason of coding style issues, if any).

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Milan Crha]

Created attachment 231631 [details]
math.h related changes

mingw32 math.h doesn't always offer M_SQRT2 and M_PI_2, thus this extends Source/WTF/wtf/MathExtras.h and applies the changes accordingly to the code.

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Milan Crha]

Created attachment 231632 [details]
miscellaenous changes

This is the last set, quite miscellaneous changes related to:
a) Source/WTF/wtf/Atomics.h - do not use MSVC code for mingw32
b) missing WTF/WTFHeaderDetection.h
c) PLATFORM(WIN) doesn't seem to work the same as OS(WINDOWS), but as long as BinarySemaphore.h uses OS(WINDOWS), the BinarySemaphore.cpp should as well (and it works fine if it does)
d) PluginPackageNone.cpp/PluginViewNone.cpp might not be used on windows, win32 build has its own implementation for these
e) missing #include <windows.h> in Source/WebCore/platform/gtk/FileSystemGtk.cpp
f) Source/WebCore/rendering/RenderBlock.cpp - the check for structure size fails on mingw32/msys, the enum size or the split of bit fields make the structure larger than with a 32bit uint, thus I both moved the bit fields to be in one group, and then added enum into the "control" structure

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Alberto Garcia]

(In reply to comment #9)
> a) Source/WTF/wtf/Atomics.h - do not use MSVC code for mingw32

Did you try using the non-Windows asm code instead? (the one
immediately on top)

> f) Source/WebCore/rendering/RenderBlock.cpp - the check for
> structure size fails on mingw32/msys, the enum size or the split of
> bit fields make the structure larger than with a 32bit uint, thus I
> both moved the bit fields to be in one group, and then added enum
> into the "control" structure

> --- webkitgtk-2.4.1.old/Source/WebCore/rendering/RenderBlock.h	2014-05-16 19:07:53 +0000
> +++ webkitgtk-2.4.1/Source/WebCore/rendering/RenderBlock.h	2014-05-17 07:38:01 +0000
> @@ -613,8 +613,8 @@ public:
>      unsigned m_beingDestroyed : 1;
>      unsigned m_hasMarkupTruncation : 1;
>      unsigned m_hasBorderOrPaddingLogicalWidthChanged : 1;
> -    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };
>      unsigned m_lineLayoutPath : 2;
> +    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };

You mean that the way this is declared doesn't allow the compiler to
use one single uint32?

> --- webkitgtk-2.4.1.old/Source/WebCore/rendering/RenderBlock.cpp	2014-05-16 19:07:53 +0000
> +++ webkitgtk-2.4.1/Source/WebCore/rendering/RenderBlock.cpp	2014-05-17 07:38:01 +0000
> @@ -83,6 +83,7 @@ using namespace HTMLNames;
>
>  struct SameSizeAsRenderBlock : public RenderBox {
>      uint32_t bitfields;
> +    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };
>  };

I don't understand why you need to declare that enum there again, what
happens if you don't do it?

[Milan Crha]

(In reply to comment #11)
> (In reply to comment #9)
> > a) Source/WTF/wtf/Atomics.h - do not use MSVC code for mingw32
> 
> Did you try using the non-Windows asm code instead? (the one
> immediately on top)

That one compiles too, when using gcc. I cannot tell how with the runtime, because I do not know what to do to trigger this particular code part, but gcc itself is happy with it. My change in the Atomics.h was (on top of the previous patch):

--- webkitgtk-2.4.1.old/Source/WTF/wtf/Atomics.h	2014-05-18 16:44:51 +0000
+++ webkitgtk-2.4.1/Source/WTF/wtf/Atomics.h	2014-05-24 12:31:40 +0000
@@ -260,7 +260,7 @@ inline void memoryBarrierBeforeUnlock()
 inline bool weakCompareAndSwap(uint8_t* location, uint8_t expected, uint8_t newValue)
 {
 #if ENABLE(COMPARE_AND_SWAP)
-#if !OS(WINDOWS) && (CPU(X86) || CPU(X86_64))
+#if COMPILER(GCC) && (CPU(X86) || CPU(X86_64))
     unsigned char result;
     asm volatile(
         "lock; cmpxchgb %3, %2\n\t"
@@ -270,7 +270,7 @@ inline bool weakCompareAndSwap(uint8_t*
         : "memory"
         );
     return result;
-#elif OS(WINDOWS) && CPU(X86) && !defined(__MINGW32__)
+#elif OS(WINDOWS) && CPU(X86)
     // FIXME: We need a 64-bit ASM implementation, but this cannot be inline due to
     // Microsoft's decision to exclude it from the compiler.
     bool result = false;

[Milan Crha]

Created attachment 232026 [details]
test.cpp for class sizes

(In reply to comment #11)
> > --- webkitgtk-2.4.1.old/Source/WebCore/rendering/RenderBlock.h	2014-05-16 19:07:53 +0000
> > +++ webkitgtk-2.4.1/Source/WebCore/rendering/RenderBlock.h	2014-05-17 07:38:01 +0000
> > @@ -613,8 +613,8 @@ public:
> >      unsigned m_beingDestroyed : 1;
> >      unsigned m_hasMarkupTruncation : 1;
> >      unsigned m_hasBorderOrPaddingLogicalWidthChanged : 1;
> > -    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };
> >      unsigned m_lineLayoutPath : 2;
> > +    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };
> 
> You mean that the way this is declared doesn't allow the compiler to
> use one single uint32?

Correct. The version a used gcc in mingw32 in 4.8.1. I wrote this little test.cpp to see what sizes are reported by gcc and the mingw32 shows:
   sizes: Base:1 Sub:8 Sub1:4 Sub2:4 Sub3:4 Sub4:4
while my linux version of gcc 4.8.2 shows:
   sizes: Base:1 Sub:4 Sub1:4 Sub2:4 Sub3:4 Sub4:4
thus the windows windows version has a problem to "join" the bits when the properties are divided with an enum (see the test.cpp for all the variants).

> > --- webkitgtk-2.4.1.old/Source/WebCore/rendering/RenderBlock.cpp	2014-05-16 19:07:53 +0000
> > +++ webkitgtk-2.4.1/Source/WebCore/rendering/RenderBlock.cpp	2014-05-17 07:38:01 +0000
> > @@ -83,6 +83,7 @@ using namespace HTMLNames;
> >
> >  struct SameSizeAsRenderBlock : public RenderBox {
> >      uint32_t bitfields;
> > +    enum LineLayoutPath { UndeterminedPath, SimpleLinesPath, LineBoxesPath, ForceLineBoxesPath };
> >  };
> 
> I don't understand why you need to declare that enum there again, what
> happens if you don't do it?

I thought it's needed, but you are right, this particular change is not required at all (see the test.cpp).

[Alberto Garcia]

(In reply to comment #12)
> That one compiles too, when using gcc.

>  #if ENABLE(COMPARE_AND_SWAP)
> -#if !OS(WINDOWS) && (CPU(X86) || CPU(X86_64))
> +#if COMPILER(GCC) && (CPU(X86) || CPU(X86_64))
>      unsigned char result;

I don't think that's good since there are more compilers that support
that syntax (e.g. clang).

I think something like !(OS(WINDOWS) && COMPILER(MSVC)) is probably
better, can you test it?

[Alberto Garcia]

(In reply to comment #14)
> I think something like !(OS(WINDOWS) && COMPILER(MSVC)) is probably
> better, can you test it?

Well, or !COMPILER(MSVC) directly, since MSVC already implies Windows.

[Milan Crha]

(In reply to comment #15)
> Well, or !COMPILER(MSVC) directly, since MSVC already implies Windows.

Yup, that compiles the asm in mingw32 too. (I've just tested it.)

[Alberto Garcia]

(In reply to comment #16)
> > Well, or !COMPILER(MSVC) directly, since MSVC already implies Windows.
>
> Yup, that compiles the asm in mingw32 too. (I've just tested it.)

Great, I'll upload the updated patch, but I have one last question:

--- a/Source/WTF/wtf/threads/BinarySemaphore.cpp
+++ b/Source/WTF/wtf/threads/BinarySemaphore.cpp
@@ -26,7 +26,7 @@
 #include "config.h"
 #include "BinarySemaphore.h"
 
-#if !PLATFORM(WIN)
+#if !OS(WINDOWS)
 

I understand that in your case you have OS(WINDOWS) but PLATFORM(GTK),
so with this change you would not be compiling BinarySemaphore.cpp, so
where's the implementation of BinarySemaphore that you are using?

BinarySemaphoreWin.cpp is not in any makefile. What's the exact error
message that you are having?

[Milan Crha]

(In reply to comment #17)
> I understand that in your case you have OS(WINDOWS) but PLATFORM(GTK),
> so with this change you would not be compiling BinarySemaphore.cpp, so
> where's the implementation of BinarySemaphore that you are using?
>
> BinarySemaphoreWin.cpp is not in any makefile. What's the exact error
> message that you are having?

I'm using BinarySemaphore.cpp too. The thing is that the BinarySemaphore.h uses OS(WINDOWS), thus the .cpp file should as well. The exact error is below. "Syncing" the check with the header file make sit compile.

  CXX      Source/WTF/wtf/text/libWTF_la-StringImpl.lo
  CXX      Source/WTF/wtf/threads/libWTF_la-BinarySemaphore.lo
Source/WTF/wtf/threads/BinarySemaphore.cpp: In constructor 'WTF::BinarySemaphore::BinarySemaphore()':
Source/WTF/wtf/threads/BinarySemaphore.cpp:34:7: error: class 'WTF::BinarySemaphore' does not have any field named 'm_isSet'
     : m_isSet(false)
       ^
Source/WTF/wtf/threads/BinarySemaphore.cpp: In member function 'void WTF::BinarySemaphore::signal()':
Source/WTF/wtf/threads/BinarySemaphore.cpp:44:24: error: 'm_mutex' was not declared in this scope
     MutexLocker locker(m_mutex);
                        ^
Source/WTF/wtf/threads/BinarySemaphore.cpp:46:5: error: 'm_isSet' was not declared in this scope
     m_isSet = true;
     ^
Source/WTF/wtf/threads/BinarySemaphore.cpp:47:5: error: 'm_condition' was not declared in this scope
     m_condition.signal();
     ^
Source/WTF/wtf/threads/BinarySemaphore.cpp: In member function 'bool WTF::BinarySemaphore::wait(double)':
Source/WTF/wtf/threads/BinarySemaphore.cpp:52:24: error: 'm_mutex' was not declared in this scope
     MutexLocker locker(m_mutex);
                        ^
Source/WTF/wtf/threads/BinarySemaphore.cpp:55:13: error: 'm_isSet' was not declared in this scope
     while (!m_isSet) {
             ^
Source/WTF/wtf/threads/BinarySemaphore.cpp:56:21: error: 'm_condition' was not declared in this scope
         timedOut = !m_condition.timedWait(m_mutex, absoluteTime);
                     ^
Source/WTF/wtf/threads/BinarySemaphore.cpp:62:5: error: 'm_isSet' was not declared in this scope
     m_isSet = false;
     ^
make: *** [Source/WTF/wtf/threads/libWTF_la-BinarySemaphore.lo] Error 1

[Alberto Garcia]

(In reply to comment #18)
> I'm using BinarySemaphore.cpp too. The thing is that the
> BinarySemaphore.h uses OS(WINDOWS), thus the .cpp file should as
> well. The exact error is below. "Syncing" the check with the header
> file make sit compile.

> Source/WTF/wtf/threads/BinarySemaphore.cpp:44:24: error: 'm_mutex' was not declared in this scope
>      MutexLocker locker(m_mutex);

I actually think you should replace OS(WINDOWS) in BinarySemaphore.h
with PLATFORM(WINDOWS), does that work?

[Milan Crha]

(In reply to comment #19)
> I actually think you should replace OS(WINDOWS) in BinarySemaphore.h
> with PLATFORM(WINDOWS), does that work?

I can change the header to use PLATFORM(WIN), yes, but then the semaphore will be a void, while the code seems to be meant to be used on windows and it compiles fine (after fixing the .cpp's file check).

More importantly, how do I test the change? I'm currently only testing whether it compiles, but nothing else, because I do not know how to trigger this code part. According to its name, it looks like a synchronization primitive, which might not be left with an "empty" implementation, otherwise other things would break badly, no? I do not know the webkitgtk code at all, though I still think it would be better to compile the implementation than to skip it.

[Alberto Garcia]

(In reply to comment #20)
> > I actually think you should replace OS(WINDOWS) in
> > BinarySemaphore.h with PLATFORM(WINDOWS), does that work?
> I can change the header to use PLATFORM(WIN), yes, but then the
> semaphore will be a void, while the code seems to be meant to be
> used on windows and it compiles fine (after fixing the .cpp's file
> check).

Are you sure it would be void?

There are two implementation of BinarySemaphore:

 - One uses CreateEvent / WaitForSingleObject from the Windows API and
   goes in BinarySemaphoreWin.cpp.

 - One uses WebKit's own Mutex class which has implementations in
   ThreadingPthreads.cpp and ThreadingWin.cpp. This goes in
   BinarySemaphore.cpp

The OS(WINDOWS) condition in the header is used to choose between
those two, but you also have to use BinarySemaphore.cpp or
BinarySemaphoreWin.cpp depending on the case.

BinarySemaphoreWin.cpp is not even shipped in the webkitgtk tarballs,
but I think BinarySemaphore.cpp should work fine.

> More importantly, how do I test the change?

This is used by the WebProcess for IPC, just run the MiniBrowser and
browser a couple of websites and you will see the WebProcess calling
WTF::BinarySemaphore::wait() all the time.

But I wonder what's the implementation that you are using if you use
#if !OS(WINDOWS) in BinarySemaphore.cpp, because I don't see any
other.

[Milan Crha]

(In reply to comment #19)
> I actually think you should replace OS(WINDOWS) in BinarySemaphore.h
> with PLATFORM(WINDOWS), does that work?

So I did so (using PLATFORM(WIN)) and it compiles and GtkLauncher.exe opens google.com page, where I can issue a search. So far so good.

[Alberto Garcia]

Created attachment 232287 [details]
miscellaneous changes

There it goes. I'll review the rest now.

[Alberto Garcia]

Comment on attachment 231631 [details]
math.h related changes

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

> webkitgtk-2.4.1/Source/WTF/wtf/MathExtras.h:75
> +#ifndef M_SQRT2
> +const double sqrtOfTwoDouble = 1.41421356237309504880;
> +#else
> +const double sqrtOfTwoDouble = M_SQRT2;
> +#endif
> +

[...]

> webkitgtk-2.4.1/Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:75
> -            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * float(M_SQRT2);
> -            m_maxPreferredLogicalWidth = maxPreferredLogicalWidth() * float(M_SQRT2);
> +            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * float(sqrtOfTwoDouble);
> +            m_maxPreferredLogicalWidth = maxPreferredLogicalWidth() * float(sqrtOfTwoDouble);

This one looks fine, maybe we can define sqrtOfTwoFloat in MathExtras.h like we do with M_PI and friends and use it here.

[Tomek Wasilczyk]

Created attachment 233527 [details]
assembler-related fixes

[Tomek Wasilczyk]

Created attachment 233528 [details]
win32-style format modifiers

[Tomek Wasilczyk]

Created attachment 233529 [details]
various small mingw32-related fixes

[Tomek Wasilczyk]

Created attachment 233530 [details]
warnings fixes

[Tomek Wasilczyk]

I've been trying to build webkitgtk 2.4.3 with mingw (using OBS and the following repository: https://build.opensuse.org/project/show/windows:mingw:win32).

I have managed to build it without JIT support, but it crashes on start (see NEXT_INSTRUCTION commented out in the "assembler-related fixes" patch). I tried building with JIT, but the symbol callToJavaScript is missing. It's defined for MSVC assembler, not gcc.

I just gave up trying, but maybe these patches will help fixing this issue.

[Milan Crha]

(In reply to comment #29)
> I tried building with JIT, but the symbol callToJavaScript is missing. It's
> defined for MSVC assembler, not gcc.

If you check the "Depends on" above, then you'll find more than this bug report fixes, I split them. The bug #132856 contains assembler implementation of the missing functions for gcc.

> +#if OS(WINDOWS)
> +        bufferPrintf("#0x%I64x", immediate);
> +#else
>         bufferPrintf("#0x%llx", immediate);
> +#endif

This is wrong, I64 is an MSVC format specifier, not gcc, thus OS(WINDOWS) check is not sufficient for correct behaviour.

Some of your changes clashes with those mine, either here, or from the "Depends On" bugs. I believe having a clean patches only with newly added changes will help with application of them.

[Milan Crha]

By the way, a complete patch I use for mingw32/msys can be found here:
https://git.gnome.org/browse/evolution/tree/win32/patches/webkitgtk.patch
It covers bug #132856 too (and the other from 'Depends on' section). This patch is applied on WebKitGTK 2.4.4. Pity all the patches here are a mess now.

[Alberto Garcia]

(In reply to comment #31)
> By the way, a complete patch I use for mingw32/msys can be found here:
> https://git.gnome.org/browse/evolution/tree/win32/patches/webkitgtk.patch
> It covers bug #132856 too (and the other from 'Depends on' section). This patch is applied on WebKitGTK 2.4.4. Pity all the patches here are a mess now.

My bad, I started reviewing these and I left the work in the middle. I don't know if I'll have the time to look into this any time soon, but I'll try to. Of course if anyone wants to step up and integrate these patches feel free!

[LRN]

About attachment 233528 [details]

Bear in mind that the use of printf format depends on two things:
1) gcc's ability to check format strings.
2) printf implementation support for various format specifiers

The (1) depends on how the function is decorated (ms_printf or gnu_printf). The impact is minor (gcc will warn about unsupported format specifiers, but will compile anyway, unless you're using -Werror)

The (2) depends on whether you define __USE_MINGW_ANSI_STDIO or not.
If you do, internal mingw printf implementation is used. That implementation supports most GNU printf format specifiers, as well as MS specifiers. It's also decorated with gnu_printf, so even though it will work with MS format specifiers at runtime, gcc will complain if you use them.
If you don't, msvcrt implementation is used. It supports (almost) none of the GNU printf format specifiers, and its standard support is poor. It is not decorated, and gcc won't check the format string.

Make up your mind, then use appropriate format strings.

[Carlos Garcia Campos]

Ok, I want to to help, but there are 9 patches here and I have no idea which ones to apply nor the order, and I don't have a windows to try them out either.

[Carlos Garcia Campos]

Ok, let's make this a meta bug, with the goal of making 2.4 branch build again in windows (WebKit1 only, of course). Please, open a new bug report for every patch required, adding it as a dependency here. Once this is fixed, I'll make 2.4.9 release.

[Milan Crha]

Created attachment 251069 [details]
preprocessor.pm / make_names.pl changes

I use this with webkit-2.4 branch at revision 182543

[Milan Crha]

I'd say all the patches here are obsolete, replaced/split into "Depends on" bugs. I do not use all of them here, my build is also limited, I use these configure options:

--enable-win32-target --enable-spellcheck --enable-jit --disable-geolocation --disable-video --disable-web-audio --disable-webgl --disable-accelerated-compositing --disable-glx --disable-egl --disable-gles2 --disable-webkit2 --disable-gtk-doc --disable-gtk-doc-html --disable-gtk-doc-pdf

Feel free to let me know when you'd like to test the webkit-2.4 branch in my environment, with any/all applied patches once you've them chosen.

[LRN]

(In reply to comment #36)
> Created attachment 251069 [details]
> preprocessor.pm / make_names.pl changes
> 
> I use this with webkit-2.4 branch at revision 182543

I've considered using it, but ended up using my own version in https://bugs.webkit.org/show_bug.cgi?id=143765

The reason is that looking for "gcc" in PATH might work (and does work for me personally), but only because mingw-gcc package provides "gcc.exe" as an alias for things like "i686-w64-mingw32-gcc-4.8.2.exe" (and more often than not the package maintainer has to make that alias deliberately, and if the package maintainer does not (by choice or omission), you won't have it). Doing such things in autotools-based build system, where configure script already detected correct $CC for us, is not a good strategy.

[Carlos Garcia Campos]

2.4.9 has been released and both Milan and LRN confirmed it built in windows. So, I'm closing this now, since the pending patches are just compile warnings that don't block this anymore. Thank you all for your help.