Bug 130585

Summary: [GTK] [EFL] Build fails with GCC < 4.8.x
Product: WebKit Reporter: Andres Gomez Garcia <agomez>
Component: PlatformAssignee: Andres Gomez Garcia <agomez>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, andersca, benjamin, berto, bunhere, calvaris, cdumez, cgarcia, clopez, cmarcelo, commit-queue, dbates, dpino, eric.carlson, eric, glenn, gustavo, gyuyoung.kim, jer.noble, kling, menard, mrobinson, philipj, pnormand, rakuco, sergio, vjaquez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 130078    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mrobinson: review-
Patch
andersca: review+
Patch
none
Patch none

Description Andres Gomez Garcia 2014-03-21 05:58:56 PDT
(webkit)tanty@pomeron:~/webkit/WebKit.git (git::master)$ Tools/Scripts/build-webkit --release --gtk --update-gtk --cmakeargs="-GNinja -j8" --makeargs="-j8"
...
[77/5629] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/MainThreadGtk.cpp.o
FAILED: /usr/lib/ccache/c++   -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DBUILDING_WTF -DDATA_DIR=\"share\" -DENABLE_3D_RENDERING=1 -DGETTEXT_PACKAGE=\"WebKitGTK-3.0\" -DHAVE_CONFIG_H=1 -DMOZ_X11 -DUSER_AGENT_GTK_MAJOR_VERSION=537 -DUSER_AGENT_GTK_MINOR_VERSION=30 -DWEBKITGTK_API_VERSION_STRING=\"3.0\" -DWTF_PLATFORM_X11=1 -DWTF_USE_3D_GRAPHICS=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DWTF_USE_EGL=1 -DWTF_USE_GLX=1 -DWTF_USE_GSTREAMER -DWTF_USE_LEVELDB=1 -DWTF_USE_OPENGL=1 -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1 -DWTF_USE_WEBAUDIO_GSTREAMER -DXP_UNIX -std=c++11 -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-omit-frame-pointer -fno-tree-dce -O3 -DNDEBUG -I../../Source/WTF -I../../Source/WTF/wtf -I../../Source/WTF/wtf/dtoa -I../../Source/WTF/wtf/threads -I../../Source/WTF/wtf/unicode -I../../Source/ThirdParty -I. -I../Dependencies/Root/include/glib-2.0 -I../Dependencies/Root/lib64/glib-2.0/include    -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/MainThreadGtk.cpp.o -MF "Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/MainThreadGtk.cpp.o.d" -o Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/MainThreadGtk.cpp.o -c ../../Source/WTF/wtf/gtk/MainThreadGtk.cpp
../../Source/WTF/wtf/gtk/MainThreadGtk.cpp: In function ‘void WTF::scheduleDispatchFunctionsOnMainThread()’:
../../Source/WTF/wtf/gtk/MainThreadGtk.cpp:43:133: error: call of overloaded ‘schedule(const char [41], void (&)())’ is ambiguous
../../Source/WTF/wtf/gtk/MainThreadGtk.cpp:43:133: note: candidates are:
In file included from ../../Source/WTF/wtf/gtk/MainThreadGtk.cpp:33:0:
../../Source/WTF/wtf/gobject/GMainLoopSource.h:53:10: note: void WTF::GMainLoopSource::schedule(const char*, std::function<void()>, int, std::function<void()>, GMainContext*)
In file included from ../../Source/WTF/wtf/gtk/MainThreadGtk.cpp:33:0:
../../Source/WTF/wtf/gobject/GMainLoopSource.h:54:10: note: void WTF::GMainLoopSource::schedule(const char*, std::function<bool()>, int, std::function<void()>, GMainContext*)
[77/5629] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/RunLoopGtk.cpp.o
FAILED: /usr/lib/ccache/c++   -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DBUILDING_WTF -DDATA_DIR=\"share\" -DENABLE_3D_RENDERING=1 -DGETTEXT_PACKAGE=\"WebKitGTK-3.0\" -DHAVE_CONFIG_H=1 -DMOZ_X11 -DUSER_AGENT_GTK_MAJOR_VERSION=537 -DUSER_AGENT_GTK_MINOR_VERSION=30 -DWEBKITGTK_API_VERSION_STRING=\"3.0\" -DWTF_PLATFORM_X11=1 -DWTF_USE_3D_GRAPHICS=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DWTF_USE_EGL=1 -DWTF_USE_GLX=1 -DWTF_USE_GSTREAMER -DWTF_USE_LEVELDB=1 -DWTF_USE_OPENGL=1 -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1 -DWTF_USE_WEBAUDIO_GSTREAMER -DXP_UNIX -std=c++11 -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-omit-frame-pointer -fno-tree-dce -O3 -DNDEBUG -I../../Source/WTF -I../../Source/WTF/wtf -I../../Source/WTF/wtf/dtoa -I../../Source/WTF/wtf/threads -I../../Source/WTF/wtf/unicode -I../../Source/ThirdParty -I. -I../Dependencies/Root/include/glib-2.0 -I../Dependencies/Root/lib64/glib-2.0/include    -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/RunLoopGtk.cpp.o -MF "Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/RunLoopGtk.cpp.o.d" -o Source/WTF/wtf/CMakeFiles/WTF.dir/gtk/RunLoopGtk.cpp.o -c ../../Source/WTF/wtf/gtk/RunLoopGtk.cpp
../../Source/WTF/wtf/gtk/RunLoopGtk.cpp: In member function ‘void WTF::RunLoop::wakeUp()’:
../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:104:31: error: call of overloaded ‘schedule(const char [22], std::_Bind_helper<false, void (WTF::RunLoop::*)(), WTF::RunLoop* const>::type, int, WTF::RunLoop::wakeUp()::<lambda()>)’ is ambiguous
../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:104:31: note: candidates are:
In file included from ../../Source/WTF/wtf/RunLoop.h:40:0,
                 from ../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:28:
../../Source/WTF/wtf/gobject/GMainLoopSource.h:53:10: note: void WTF::GMainLoopSource::schedule(const char*, std::function<void()>, int, std::function<void()>, GMainContext*)
In file included from ../../Source/WTF/wtf/RunLoop.h:40:0,
                 from ../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:28:
../../Source/WTF/wtf/gobject/GMainLoopSource.h:54:10: note: void WTF::GMainLoopSource::schedule(const char*, std::function<bool()>, int, std::function<void()>, GMainContext*)
[77/5629] Building CXX object Source/WebCore/CMakeFiles/ANGLESupport.dir/__/ThirdParty/ANGLE/src/compiler/translator/SearchSymbol.cpp.o
ninja: build stopped: subcommand failed.
Comment 1 Andres Gomez Garcia 2014-03-21 06:00:12 PDT
This error happens after fixing bug 130027:
http://trac.webkit.org/changeset/165952
Comment 2 Andres Gomez Garcia 2014-03-21 06:01:41 PDT
(webkit)tanty@pomeron:~/webkit/WebKit.git (git::master)$ gcc --version
gcc (Ubuntu/Linaro 4.7.3-1ubuntu1) 4.7.3
Copyright (C) 2012 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Comment 3 Andres Gomez Garcia 2014-03-21 06:03:09 PDT
The behavior is explained at:
http://stackoverflow.com/questions/22146749/overload-resolution-with-stdfunction

And seems to be related to:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2132

Possible options are:
 * Provide a explicit cast for the failing overloads
 * Bump the mimum GCC version up to 4.8.x
Comment 4 Andres Gomez Garcia 2014-03-21 06:10:48 PDT
Created attachment 227423 [details]
Patch
Comment 5 Andres Gomez Garcia 2014-03-21 06:39:25 PDT
Created attachment 227425 [details]
Patch
Comment 6 Andres Gomez Garcia 2014-03-21 06:40:10 PDT
(In reply to comment #5)
> Created an attachment (id=227425) [details]
> Patch

This is an alternative patch just bumping GCC version to 4.8.0
Comment 7 Zan Dobersek 2014-03-21 06:43:58 PDT
Comment on attachment 227423 [details]
Patch

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

> Source/WTF/wtf/gtk/MainThreadGtk.cpp:43
> -    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] dispatchFunctionsFromMainThread", dispatchFunctionsFromMainThread);
> +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] dispatchFunctionsFromMainThread", (std::function<void()>) dispatchFunctionsFromMainThread);

Using the std::function<void ()> constructor would be more in line with the coding style than the C-style cast:

    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] dispatchFunctionsFromMainThread", std::function<void ()>(dispatchFunctionsFromMainThread));

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:103
> -    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this),
> +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", (std::function<void()>) std::bind(&RunLoop::performWork, this),

Ditto.
Comment 8 Carlos Garcia Campos 2014-03-21 06:46:29 PDT
I prefer to bump the gcc requirements than adding unnecessary casts to workaround the issue, even more considering that it doesn't fail to compile with clang either.
Comment 9 Zan Dobersek 2014-03-21 06:47:46 PDT
I'm of indifferent opinion here. We'll bump to GCC 4.8.0 at some point, so we might as well do it now if there's consent for that. If EFL can't yet commit to bumping, we can only deploy this requirement for the GTK port.

But I'm fine with the changes in attachment #227423 [details] as well.
Comment 10 Andres Gomez Garcia 2014-03-21 06:59:41 PDT
Created attachment 227428 [details]
Patch
Comment 11 Andres Gomez Garcia 2014-03-21 07:05:47 PDT
Created attachment 227429 [details]
Patch
Comment 12 Andres Gomez Garcia 2014-03-21 07:09:52 PDT
Comment on attachment 227423 [details]
Patch

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

>> Source/WTF/wtf/gtk/MainThreadGtk.cpp:43
>> +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] dispatchFunctionsFromMainThread", (std::function<void()>) dispatchFunctionsFromMainThread);
> 
> Using the std::function<void ()> constructor would be more in line with the coding style than the C-style cast:
> 
>     GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] dispatchFunctionsFromMainThread", std::function<void ()>(dispatchFunctionsFromMainThread));

You are right, I just messed up because of reading http://www.stroustrup.com/C++11FAQ.html#std-function and the examples for expliciting the overloaded functions in the std::bind examples O:)

>> Source/WTF/wtf/gtk/RunLoopGtk.cpp:103
>> +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", (std::function<void()>) std::bind(&RunLoop::performWork, this),
> 
> Ditto.

Done.
Comment 13 Andres Gomez Garcia 2014-03-21 07:18:02 PDT
Comment on attachment 227428 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:311
> +        message(FATAL_ERROR "You need at least GCC 4.8.0 and current version is ${CMAKE_CXX_COMPILER_VERSION}")

Modified the previous patch so this error condition only happens for the GTK port.

Now, it seems that there is shared GStreamer code with the EFL port that would fail also because of the same reason:
https://bugs.webkit.org/show_bug.cgi?id=130078

Hence, I suppose the original patch was actually better than this, now.
Comment 14 Martin Robinson 2014-03-21 07:20:41 PDT
Comment on attachment 227428 [details]
Patch

We cannot bump our dependencies until the next major release. Seems silly to do it for something that has a trivial workaround too.
Comment 15 Andres Gomez Garcia 2014-03-21 07:36:22 PDT
(In reply to comment #14)
> (From update of attachment 227428 [details])
> We cannot bump our dependencies until the next major release. Seems silly to do it for something that has a trivial workaround too.

This is a building dependency, not a running one, so I'm not sure it would then be a reason to wait until a major release.

In any case, I'm all for going with explicit casts :)
Comment 16 Carlos Garcia Campos 2014-03-21 07:42:47 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 227428 [details] [details])
> > We cannot bump our dependencies until the next major release. Seems silly to do it for something that has a trivial workaround too.
> 
> This is a building dependency, not a running one, so I'm not sure it would then be a reason to wait until a major release.

Exactly.
 
> In any case, I'm all for going with explicit casts :)

I would add a FIXME or something, since it's a workaround that works with newer gcc and clang. I prefer to keep the code clone and less confusing and bump the gcc requirements.
Comment 17 Martin Robinson 2014-03-21 07:44:15 PDT
(In reply to comment #16)

> > This is a building dependency, not a running one, so I'm not sure it would then be a reason to wait until a major release.
> 
> Exactly.

My understanding is that if you cannot build it on old Debian, you cannot ship it on old Debian. Perhaps that's not how it works though?
Comment 18 Anders Carlsson 2014-03-22 17:49:55 PDT
Comment on attachment 227429 [details]
Patch

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

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:104
> +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::function<void()>(std::bind(&RunLoop::performWork, this)),
>          G_PRIORITY_DEFAULT, [this] { deref(); });

Please use a lambda with a RefPtr capture here instead of calling ref and deref.
Comment 19 Carlos Garcia Campos 2014-03-24 03:10:41 PDT
(In reply to comment #18)
> (From update of attachment 227429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227429&action=review
> 
> > Source/WTF/wtf/gtk/RunLoopGtk.cpp:104
> > +    GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::function<void()>(std::bind(&RunLoop::performWork, this)),
> >          G_PRIORITY_DEFAULT, [this] { deref(); });
> 
> Please use a lambda with a RefPtr capture here instead of calling ref and deref.

I thought about it, but then the lambda does nothing and it looked more confusing to me, is that preferred?

I guess it would be something like

RefPtr<RunLoop> protector(this);
GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::function<void()>(std::bind(&RunLoop::performWork, this)), [protector] {});

right?
Comment 20 Alberto Garcia 2014-03-24 03:20:10 PDT
(In reply to comment #17)
> My understanding is that if you cannot build it on old Debian, you
> cannot ship it on old Debian. Perhaps that's not how it works
> though?

I don't think there's any problem from the Debian side since we
already switched to gcc 4.8 in both the 2.2 and the 2.4 branches.

The problem would be in Debian stable, but I think there are other
dependencies missing, so I wouldn't bother too much.
Comment 21 Carlos Garcia Campos 2014-03-24 03:38:13 PDT
(In reply to comment #20)
> (In reply to comment #17)
> > My understanding is that if you cannot build it on old Debian, you
> > cannot ship it on old Debian. Perhaps that's not how it works
> > though?
> 
> I don't think there's any problem from the Debian side since we
> already switched to gcc 4.8 in both the 2.2 and the 2.4 branches.
> 
> The problem would be in Debian stable, but I think there are other
> dependencies missing, so I wouldn't bother too much.

And EFL guys already agreed on upgrading their bots to use gcc 4.8, they are working on it, so it will happen soon. I still prefer to bump the gcc requirements instead of adding unnecessary casts as a workaround.
Comment 22 Anders Carlsson 2014-03-25 08:11:02 PDT
> 
> I guess it would be something like
> 
> RefPtr<RunLoop> protector(this);
> GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::function<void()>(std::bind(&RunLoop::performWork, this)), [protector] {});

Why is the second parameter even needed? Can't you do something like

RefPtr<RunLoop> runLoop(this);
GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", [runLoop] {
    runLoop->performWork();
});

As long as the function object is destroyed at the right time, no extra parameter is needed.
Comment 23 Carlos Garcia Campos 2014-03-25 08:20:26 PDT
(In reply to comment #22)
> > 
> > I guess it would be something like
> > 
> > RefPtr<RunLoop> protector(this);
> > GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", std::function<void()>(std::bind(&RunLoop::performWork, this)), [protector] {});
> 
> Why is the second parameter even needed? Can't you do something like
> 
> RefPtr<RunLoop> runLoop(this);
> GMainLoopSource::createAndDeleteOnDestroy().schedule("[WebKit] RunLoop work", [runLoop] {
>     runLoop->performWork();
> });
> 
> As long as the function object is destroyed at the right time, no extra parameter is needed.

Yes, this would work for this particular case, since the function is detroyed right before the destroy callback.
Comment 24 Víctor M. Jáquez L. 2014-03-25 08:40:55 PDT
Not only GMainLoopSource::createAndDeleteOnDestroy().schedule() fails at casting when gcc is less than 4.8, but also, in r163536, the function std::chrono::steady_clock::now() is used and it is only supported since gcc 4.8. So either EFL and GTK ports need  gcc 4.8
Comment 25 Martin Robinson 2014-03-25 15:40:11 PDT
(In reply to comment #20)

> I don't think there's any problem from the Debian side since we
> already switched to gcc 4.8 in both the 2.2 and the 2.4 branches.
> 
> The problem would be in Debian stable, but I think there are other
> dependencies missing, so I wouldn't bother too much.

Can we guarantee this for all downstreams though?
Comment 26 Alberto Garcia 2014-03-26 04:44:29 PDT
(In reply to comment #25)
> > I don't think there's any problem from the Debian side since we
> > already switched to gcc 4.8 in both the 2.2 and the 2.4 branches.

> Can we guarantee this for all downstreams though?

I was checking a bit the status of gcc 4.8 on some major
distributions, and it seems to be available in all of them:

ubuntu: since 13.10 (saucy)
packages.ubuntu.com/gcc-4.8

fedora: since F9 (july 2013)
https://admin.fedoraproject.org/updates/gcc?_csrf_token=5612f8b4909851d73b973c8196fa79957fcf32b7

opensuse:
http://software.opensuse.org/package/gcc48

gentoo:
http://packages.gentoo.org/package/sys-devel/gcc

freebsd:
http://svnweb.freebsd.org/ports/head/lang/gcc48/
Comment 27 Alberto Garcia 2014-04-02 08:23:37 PDT
gcc 4.9 will be released soon (see http://gcc.gnu.org/ml/gcc/2014-03/msg00190.html) so I would say let's support 4.7 for the time being and require 4.8 once the new version is available.
Comment 28 Andres Gomez Garcia 2014-04-04 02:43:01 PDT
(In reply to comment #24)
> Not only GMainLoopSource::createAndDeleteOnDestroy().schedule() fails at casting when gcc is less than 4.8, but also, in r163536, the function std::chrono::steady_clock::now() is used and it is only supported since gcc 4.8. So either EFL and GTK ports need  gcc 4.8

Reading the documentation for libstdc++, not really ... steady_clock was the same than system_clock and since 4.8.1 they are different classes.

However, not having any bot with GCC < 4.8.x will make it difficult to detect when new code that is not supported is added.

I think if we want to keep supporting GCC 4.7.x we need a bot using it or we will just keep chasing changes that may break the compilation.

Anyway, I'm double checking just not.
Comment 29 Andres Gomez Garcia 2014-04-07 08:53:14 PDT
Created attachment 228738 [details]
Patch
Comment 30 Andres Gomez Garcia 2014-04-07 08:54:52 PDT
(In reply to comment #29)
> Created an attachment (id=228738) [details]
> Patch

The problem is spreading to many other places in the code.

If we really want to keep compatibility with GCC < 4.8 we will *need* a buildbot in the waterfall that would be compiliing with that version of GCC. Otherwise, new changes will get into the trunk unnoticed.
Comment 31 Andres Gomez Garcia 2014-04-07 08:57:49 PDT
(In reply to comment #28)
> (In reply to comment #24)
> > Not only GMainLoopSource::createAndDeleteOnDestroy().schedule() fails at casting when gcc is less than 4.8, but also, in r163536, the function std::chrono::steady_clock::now() is used and it is only supported since gcc 4.8. So either EFL and GTK ports need  gcc 4.8
> 
> Reading the documentation for libstdc++, not really ... steady_clock was the same than system_clock and since 4.8.1 they are different classes.

Double checked: this is NOT a problem with GCC 4.7.x
Comment 32 Andres Gomez Garcia 2014-04-07 08:58:41 PDT
Created attachment 228739 [details]
Patch
Comment 33 Andres Gomez Garcia 2014-04-07 09:01:48 PDT
(In reply to comment #32)
> Created an attachment (id=228739) [details]
> Patch

I think we only have the option of bumping GCC version without a gatekeeper to check when new code not compatible with GCC 4.7.x is added.

I'm upgrading GCC to 4.8 myself now so I won't know any more ...
Comment 34 Martin Robinson 2014-04-07 09:37:21 PDT
Comment on attachment 228738 [details]
Patch

We can do this or simply rename on of the schedule functions.
Comment 35 Carlos Garcia Campos 2014-04-07 23:58:26 PDT
(In reply to comment #34)
> (From update of attachment 228738 [details])
> We can do this or simply rename on of the schedule functions.

Please, don't rename the functions, I'm fine adding workarounds for broken compiler versions, but not changing an API for that if not required.
Comment 36 Andres Gomez Garcia 2014-04-08 00:38:08 PDT
Comment on attachment 228738 [details]
Patch

Clearing flags on attachment: 228738

Committed r166916: <http://trac.webkit.org/changeset/166916>
Comment 37 Andres Gomez Garcia 2014-04-08 00:38:23 PDT
All reviewed patches have been landed.  Closing bug.