Bug 181160 – REGRESSION(r225769): Build error with constexpr std::max // std::min in libdstdc++4

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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

[Yusuke Suzuki]

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.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

Created attachment 330191 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 330192 [details]
Patch

Try to fix Mac build: add algorithm header

[Carlos Alberto Lopez Perez]

Created attachment 330194 [details]
Patch

Try to fix Win build: disambiguate usage of min and max in UniscribeController.cpp

[Carlos Alberto Lopez Perez]

Created attachment 330198 [details]
Patch

Try to fix Win build (2): disambiguate usage of max in MediaPlayerPrivateAVFoundationCF.cpp

[Carlos Alberto Lopez Perez]

Created attachment 330200 [details]
Patch

Try to fix Win build (3): disambiguate usage of min in PluginView.cpp

[Michael Catanzaro]

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.

[Carlos Alberto Lopez Perez]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Carlos Alberto Lopez Perez]

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)

[Michael Catanzaro]

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

[Carlos Alberto Lopez Perez]

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

[Myles C. Maxfield]

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?

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

Committed r226299: <https://trac.webkit.org/changeset/226299>

[Konstantin Tokarev]

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

[Michael Catanzaro]

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

[Konstantin Tokarev]

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.

[Carlos Alberto Lopez Perez]

Committed r226300: <https://trac.webkit.org/changeset/226300>

[Carlos Alberto Lopez Perez]

Committed r226301: <https://trac.webkit.org/changeset/226301>

[Carlos Alberto Lopez Perez]

Committed r226302: <https://trac.webkit.org/changeset/226302>

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Radar WebKit Bug Importer]

<rdar://problem/36261379>