Bug 181160 - REGRESSION(r225769): Build error with constexpr std::max // std::min in libdstdc++4
Summary: REGRESSION(r225769): Build error with constexpr std::max // std::min in libds...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-26 06:23 PST by Carlos Alberto Lopez Perez
Modified: 2018-06-19 00:45 PDT (History)
19 users (show)

See Also:


Attachments
Patch (9.00 KB, patch)
2017-12-26 07:38 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2017-12-26 08:12 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (10.70 KB, patch)
2017-12-26 08:46 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (11.76 KB, patch)
2017-12-26 09:32 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (13.46 KB, patch)
2017-12-26 10:20 PST, Carlos Alberto Lopez Perez
mmaxfield: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (3.04 MB, application/zip)
2017-12-26 11:41 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2017-12-26 06:23:30 PST
Similar to what happened with std::tie in bug 180692 it seems libstdc++-4 (as shipped on Debian Jessie) doesn't have a version of std::max or std::min annotated with constexpr, so build fails after r225769 <https://trac.webkit.org/r225769>

Build failure: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/8668/steps/compile-webkit/logs/stdio


Related: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60271
Comment 1 Yusuke Suzuki 2017-12-26 06:37:24 PST
I have one question, do we need to support libstdc++4?
Our GCC baseline is now 5.2. And I think Debian jessie is not supported by WebKit ToT right now since we have Debian stretch, which is newer stable version of Debian. While std::tie's issue is not fixed in libstdc++5, this std::{max,min} is fixed in libstdc++5.
Comment 2 Carlos Alberto Lopez Perez 2017-12-26 07:37:07 PST
(In reply to Yusuke Suzuki from comment #1)
> I have one question, do we need to support libstdc++4?
> Our GCC baseline is now 5.2. And I think Debian jessie is not supported by
> WebKit ToT right now since we have Debian stretch, which is newer stable
> version of Debian. While std::tie's issue is not fixed in libstdc++5, this
> std::{max,min} is fixed in libstdc++5.

Per WebKitGTK+ Dependencies Policy https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy we should try to support Debian stable releases up to one year after the next release (even when that means using a non-default toolchain).

Debian 9 (Stretch) was released on 2017-06. Which means that we should try to support Debian 8 (Jessie) up to 2018-06.

For that reason the Debian stable bot is still running Debian 8 (Jessie). Since GCC-4.9 is not modern enough to build WebKit, it is using as compiler clang-3.8 from the repository jessie-backports.
Comment 3 Carlos Alberto Lopez Perez 2017-12-26 07:38:25 PST
Created attachment 330191 [details]
Patch
Comment 4 Carlos Alberto Lopez Perez 2017-12-26 08:12:41 PST
Created attachment 330192 [details]
Patch

Try to fix Mac build: add algorithm header
Comment 5 Carlos Alberto Lopez Perez 2017-12-26 08:46:39 PST
Created attachment 330194 [details]
Patch

Try to fix Win build: disambiguate usage of min and max in UniscribeController.cpp
Comment 6 Carlos Alberto Lopez Perez 2017-12-26 09:32:47 PST
Created attachment 330198 [details]
Patch

Try to fix Win build (2): disambiguate usage of max in MediaPlayerPrivateAVFoundationCF.cpp
Comment 7 Carlos Alberto Lopez Perez 2017-12-26 10:20:11 PST
Created attachment 330200 [details]
Patch

Try to fix Win build (3): disambiguate usage of min in PluginView.cpp
Comment 8 Michael Catanzaro 2017-12-26 10:25:24 PST
I don't know. libstdc++4 is *really* old. Would it be possible to change the bot to static link to libc++? There is no C++ in the WebKit API, so we don't need to worry about C++ ABI, so I don't think dynamic linking to the system libstdc++ should be required.
Comment 9 Carlos Alberto Lopez Perez 2017-12-26 11:05:19 PST
(In reply to Michael Catanzaro from comment #8)
> I don't know. libstdc++4 is *really* old. Would it be possible to change the
> bot to static link to libc++? There is no C++ in the WebKit API, so we don't
> need to worry about C++ ABI, so I don't think dynamic linking to the system
> libstdc++ should be required.

Regarding static linking WebKit: That's would be the holy grail! I have tried several times to do that (for different reasons) but I have never managed to get it working. If you know how to achieve this I'm certainly interested. Also take into account that even if you manage to do this certain dependencies of WebKit like glib/gio dlopen() libraries.

Regarding using libc++: Maybe its possible, but what would be the point? I don't think any of our downstream users is interested in building WK with libc++.

Rather than moving to libc++ I will just give up on supporting Debian jessie before the promised year and move the bot to Debian Stretch.

Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with -Wno-invalid-constexpr. That will make clang happy.
Comment 10 Build Bot 2017-12-26 11:41:30 PST
Comment on attachment 330200 [details]
Patch

Attachment 330200 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5836611

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html
Comment 11 Build Bot 2017-12-26 11:41:32 PST
Created attachment 330203 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Carlos Alberto Lopez Perez 2017-12-26 16:44:27 PST
I think the a(In reply to Build Bot from comment #10)
> Comment on attachment 330200 [details]
> Patch
> 
> Attachment 330200 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/5836611
> 
> New failing tests:
> imported/w3c/web-platform-tests/service-workers/service-worker/
> ServiceWorkerGlobalScope/update.https.html

I think this failure is totally unrelated (the EWS seems flaky)
Comment 13 Michael Catanzaro 2017-12-26 17:11:41 PST
(In reply to Carlos Alberto Lopez Perez from comment #9)
> Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with
> -Wno-invalid-constexpr. That will make clang happy.

This would be less intrusive, IMO.
Comment 14 Carlos Alberto Lopez Perez 2017-12-26 18:09:13 PST
(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Alberto Lopez Perez from comment #9)
> > Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with
> > -Wno-invalid-constexpr. That will make clang happy.
> 
> This would be less intrusive, IMO.

Unfortunately that switch is only recognized by Clang, but not by GCC, and I can't find a similar one for GCC.
Therefore doing this it will fix only the build with libstdc++-4 and Clang but not with GCC (you can in theory use a newer GCC with an old libstdc++).
Comment 15 Myles C. Maxfield 2017-12-26 21:16:24 PST
Comment on attachment 330200 [details]
Patch

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

> Source/WTF/wtf/StdLibExtras.h:470
> +template <class T>
> +inline constexpr const T& min(const T& a, const T& b)
> +{
> +    return std::min(a, b);
> +}
> +
> +template <class T>
> +inline constexpr const T& max(const T& a, const T& b)
> +{
> +    return std::max(a, b);
> +}

If the new rule will be "don't use std::min(), better to use WTF::min()" can we add a style check or commit check or something to enforce it?
Comment 16 Carlos Alberto Lopez Perez 2017-12-27 07:27:42 PST
(In reply to Myles C. Maxfield from comment #15)
> Comment on attachment 330200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330200&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:470
> > +template <class T>
> > +inline constexpr const T& min(const T& a, const T& b)
> > +{
> > +    return std::min(a, b);
> > +}
> > +
> > +template <class T>
> > +inline constexpr const T& max(const T& a, const T& b)
> > +{
> > +    return std::max(a, b);
> > +}
> 
> If the new rule will be "don't use std::min(), better to use WTF::min()" can
> we add a style check or commit check or something to enforce it?


I think this is not the new rule. The idea is to only use WTF::min when you expect std::min to return a constexpr result, but otherwise just use standard std::min.

Regarding checking/enforcing it, on one hand I think is currently enough with the Ubuntu LTS and Debian buildbots that will turn red if the build breaks on them. We are ok with carrying with the burden of fixing this after the fact.

On the other hand, this will be only a temporally situation, because on 2018-06 we will be updating this bot to the next release of Debian, so then we can officially stop supporting libstdc++-4 and we can remove this workaround from the WebKit code. I will add a note on the comments about this timing to not forget.
Comment 17 Carlos Alberto Lopez Perez 2017-12-27 08:46:12 PST
Committed r226299: <https://trac.webkit.org/changeset/226299>
Comment 18 Konstantin Tokarev 2017-12-27 08:53:12 PST
(In reply to Carlos Alberto Lopez Perez from comment #9)
> Regarding static linking WebKit: That's would be the holy grail!

Too much typos in word "sh*t" :P

Every c++ package depending on webkit will be screwed, as distro will build it against system library.
Comment 19 Michael Catanzaro 2017-12-27 09:08:21 PST
(In reply to Konstantin Tokarev from comment #18)
> (In reply to Carlos Alberto Lopez Perez from comment #9)
> > Regarding static linking WebKit: That's would be the holy grail!
> 
> Too much typos in word "sh*t" :P
> 
> Every c++ package depending on webkit will be screwed, as distro will build
> it against system library.

There is no C++ in our API, so I don't see why it would be a problem. InjectedBundle loading does use C++, but not the standard library, so that should be fine too.
Comment 20 Konstantin Tokarev 2017-12-27 09:13:56 PST
Reality is that it's impossible to mix C++ standard libraries on Linux in one application.

See e.g. http://lists.qt-project.org/pipermail/development/2017-March/029115.html, and I can probably find other email where Thiago explains the issue.
Comment 21 Carlos Alberto Lopez Perez 2017-12-27 09:29:37 PST
Committed r226300: <https://trac.webkit.org/changeset/226300>
Comment 22 Carlos Alberto Lopez Perez 2017-12-27 09:38:23 PST
Committed r226301: <https://trac.webkit.org/changeset/226301>
Comment 23 Carlos Alberto Lopez Perez 2017-12-27 09:41:28 PST
Committed r226302: <https://trac.webkit.org/changeset/226302>
Comment 24 Carlos Alberto Lopez Perez 2017-12-27 10:29:31 PST
(In reply to Michael Catanzaro from comment #19)
> (In reply to Konstantin Tokarev from comment #18)
> > (In reply to Carlos Alberto Lopez Perez from comment #9)
> > > Regarding static linking WebKit: That's would be the holy grail!
> > 
> > Too much typos in word "sh*t" :P
> > 
> > Every c++ package depending on webkit will be screwed, as distro will build
> > it against system library.
> 
> There is no C++ in our API, so I don't see why it would be a problem.
> InjectedBundle loading does use C++, but not the standard library, so that
> should be fine too.

What about someone implementing the UIProcess on C++? Even when there is no C++ in our API, there is WebKit-related C++ code under the hood on the UIProcess.
Comment 25 Carlos Alberto Lopez Perez 2017-12-27 10:31:05 PST
(In reply to Carlos Alberto Lopez Perez from comment #24)
> (In reply to Michael Catanzaro from comment #19)
> > (In reply to Konstantin Tokarev from comment #18)
> > > (In reply to Carlos Alberto Lopez Perez from comment #9)
> > > > Regarding static linking WebKit: That's would be the holy grail!
> > > 
> > > Too much typos in word "sh*t" :P
> > > 
> > > Every c++ package depending on webkit will be screwed, as distro will build
> > > it against system library.
> > 
> > There is no C++ in our API, so I don't see why it would be a problem.
> > InjectedBundle loading does use C++, but not the standard library, so that
> > should be fine too.
> 
> What about someone implementing the UIProcess on C++? Even when there is no
> C++ in our API, there is WebKit-related C++ code under the hood on the
> UIProcess.

I mean: "What about someone implementing a MiniBrowser/launcher on C++?"
Comment 26 Radar WebKit Bug Importer 2018-01-02 13:18:34 PST
<rdar://problem/36261379>