Bug 174155 - Upgrade GCC baseline
Summary: Upgrade GCC baseline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-05 03:24 PDT by Yusuke Suzuki
Modified: 2017-07-06 05:12 PDT (History)
13 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2017-07-05 04:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (1.48 KB, patch)
2017-07-05 04:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.90 KB, patch)
2017-07-05 19:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.26 KB, patch)
2017-07-05 19:23 PDT, Yusuke Suzuki
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-07-05 03:24:54 PDT
We met the conclusion that we can upgrade GCC baseline at start of July![1].

According to the Debian's package information, GCC of the current stable (stretch, recently released) is 6.3.0.
And Ubuntu 16.04's GCC is 5.4.0.

And the table of the features is here.
https://gcc.gnu.org/projects/cxx-status.html
https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html

https://gcc.gnu.org/gcc-5/changes.html
https://gcc.gnu.org/gcc-6/changes.html
https://gcc.gnu.org/gcc-7/changes.html

If we can upgrade our GCC to 5~, we have (almost) full feature of C++14 including relaxed-constexpr.
If we can upgrade our GCC to 6~, we have some part of C++17. (Like, u8 literal, extended static_assert(cond)).
And if we can upgrade our GCC to 7~, we have bunch of C++17. It includes std::optional, std::variant, std::conjunction.

[1]: https://lists.webkit.org/pipermail/webkit-dev/2017-June/029220.html
Comment 1 Yusuke Suzuki 2017-07-05 03:30:17 PDT
Even if we take very conservative version (GCC 5), it means that we have full-featured C++14 including related-constexpr. (And it is also supported in MSVC 2017).
Comment 2 Michael Catanzaro 2017-07-05 03:38:52 PDT
GTK and WPE are ready for GCC 5, except for a couple nonessential bots that are not run by Igalia. We would prefer to defer requiring GCC 6 until next summer.

AFAIK the only thing we have to change is the version number in OptionsGTK.cmake?
Comment 3 Yusuke Suzuki 2017-07-05 03:46:19 PDT
(In reply to Michael Catanzaro from comment #2)
> GTK and WPE are ready for GCC 5, except for a couple nonessential bots that
> are not run by Igalia. We would prefer to defer requiring GCC 6 until next
> summer.

Anyway, the large part of C++17 is shipped in GCC 7. So I think if we would like to use C++17, we need to upgrade GCC to 7 at least.
So, personally, GCC 5 for now seems fine to me.
It would be nice if we can upgrade our GCC to 7 next summer...

BTW, to use C++14 relaxed constexpr, we need to upgrade our MSVC to 2017 too.
I think it is ok because Windows does not have bunch of distributions which does not have the easy way to install MSVC 2017.

> AFAIK the only thing we have to change is the version number in
> OptionsGTK.cmake?

I think so.
Comment 4 Carlos Alberto Lopez Perez 2017-07-05 04:10:46 PDT
(In reply to Yusuke Suzuki from comment #3)
> (In reply to Michael Catanzaro from comment #2)
> > GTK and WPE are ready for GCC 5, except for a couple nonessential bots that
> > are not run by Igalia. We would prefer to defer requiring GCC 6 until next
> > summer.
> 
> Anyway, the large part of C++17 is shipped in GCC 7. So I think if we would
> like to use C++17, we need to upgrade GCC to 7 at least.
> So, personally, GCC 5 for now seems fine to me.
> It would be nice if we can upgrade our GCC to 7 next summer...
> 

According to https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy

* Its fine to require now at least GCC 5.3 (that is what Ubuntu 16.04 ships)

* On May of 2018 (when Ubuntu 18.04 will be released -- next LTS) it will be fine to require at least GCC 6.3 (that is what Debian 9 ships currently).

* It won't be fine to require GCC >= 7 until at least 2019-2020 (when Debian 10 will be released)


I understand the desire to use new compiler features, but at the same time I understand that requiring a too much new version of the compiler will cause trouble for many users. This is specially true for embedded users where the version of the compiler/toolchain is not something you can upgrade easier.

I think our current policy is a good compromise between using new compiler features and allowing WebKit to be built with a reasonably old toolchain.
Comment 5 Yusuke Suzuki 2017-07-05 04:44:08 PDT
Created attachment 314601 [details]
Patch
Comment 6 Yusuke Suzuki 2017-07-05 04:47:13 PDT
Created attachment 314602 [details]
Patch
Comment 7 Carlos Alberto Lopez Perez 2017-07-05 06:59:26 PDT
Comment on attachment 314602 [details]
Patch

Should we move this version check to a more generic cmake file so it applies to all ports (including WPE)? Perhaps on the top level CMakeLists.txt ?
At the same time, i think we can remove other workarounds we have for GCC < 5.3.0 like the one done on https://trac.webkit.org/changeset/184857
Comment 8 Michael Catanzaro 2017-07-05 08:22:44 PDT
(In reply to Carlos Alberto Lopez Perez from comment #7)
> Comment on attachment 314602 [details]
> Patch
> 
> Should we move this version check to a more generic cmake file so it applies
> to all ports (including WPE)? Perhaps on the top level CMakeLists.txt ?

I think the toplevel CMakeLists.txt is a reasonable location for this.

> At the same time, i think we can remove other workarounds we have for GCC <
> 5.3.0 like the one done on https://trac.webkit.org/changeset/184857

Yeah good catch. Problem is, it's hard to find these since they're scattered in different places.
Comment 9 Yusuke Suzuki 2017-07-05 19:17:58 PDT
Created attachment 314676 [details]
Patch
Comment 10 Yusuke Suzuki 2017-07-05 19:23:38 PDT
Created attachment 314679 [details]
Patch
Comment 11 Michael Catanzaro 2017-07-05 21:00:50 PDT
Comment on attachment 314679 [details]
Patch

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

> Source/WTF/wtf/Compiler.h:98
> +#if !GCC_VERSION_AT_LEAST(5, 3, 0)
> +#error "Please use a newer version of GCC. WebKit requires GCC 5.3.0 or newer to compile."

This seems pretty arbitrary. Since there's one fewer significant digit in the version now, it's like setting the requirement at 4.9.3. I would just set it at 5.0 and be done with it.

But this is fine too.
Comment 12 Yusuke Suzuki 2017-07-05 21:46:05 PDT
Comment on attachment 314679 [details]
Patch

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

>> Source/WTF/wtf/Compiler.h:98
>> +#error "Please use a newer version of GCC. WebKit requires GCC 5.3.0 or newer to compile."
> 
> This seems pretty arbitrary. Since there's one fewer significant digit in the version now, it's like setting the requirement at 4.9.3. I would just set it at 5.0 and be done with it.
> 
> But this is fine too.

It would be fine. I've changed it.
Comment 13 Yusuke Suzuki 2017-07-05 21:51:18 PDT
Committed r219186: <http://trac.webkit.org/changeset/219186>
Comment 14 Carlos Alberto Lopez Perez 2017-07-06 05:12:57 PDT
Done: Installed clang-3.8 from jessie-backports on the Debian Stable (currently: Jessie) bot and set it as compiler with the CC/CXX environment variables https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29