Bug 130585 – [GTK] [EFL] Build fails with GCC < 4.8.x

• Component: Platform
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

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

[Andres Gomez Garcia]

This error happens after fixing bug 130027:
http://trac.webkit.org/changeset/165952

[Andres Gomez Garcia]

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

[Andres Gomez Garcia]

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

[Andres Gomez Garcia]

Created attachment 227423 [details]
Patch

[Andres Gomez Garcia]

Created attachment 227425 [details]
Patch

[Andres Gomez Garcia]

(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

[Zan Dobersek]

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.

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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.

[Andres Gomez Garcia]

Created attachment 227428 [details]
Patch

[Andres Gomez Garcia]

Created attachment 227429 [details]
Patch

[Andres Gomez Garcia]

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.

[Andres Gomez Garcia]

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.

[Martin Robinson]

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.

[Andres Gomez Garcia]

(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 :)

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Anders Carlsson]

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.

[Carlos Garcia Campos]

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

[Alberto Garcia]

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

[Carlos Garcia Campos]

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

[Anders Carlsson]

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

[Carlos Garcia Campos]

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

[Víctor M. Jáquez L.]

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

[Martin Robinson]

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

[Alberto Garcia]

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

[Alberto Garcia]

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.

[Andres Gomez Garcia]

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

[Andres Gomez Garcia]

Created attachment 228738 [details]
Patch

[Andres Gomez Garcia]

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

[Andres Gomez Garcia]

(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

[Andres Gomez Garcia]

Created attachment 228739 [details]
Patch

[Andres Gomez Garcia]

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

[Martin Robinson]

Comment on attachment 228738 [details]
Patch

We can do this or simply rename on of the schedule functions.

[Carlos Garcia Campos]

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

[Andres Gomez Garcia]

Comment on attachment 228738 [details]
Patch

Clearing flags on attachment: 228738

Committed r166916: <http://trac.webkit.org/changeset/166916>

[Andres Gomez Garcia]

All reviewed patches have been landed.  Closing bug.