Bug 185198

Summary: [GTK] REGRESSION(r231170) Build broken with Clang 5.0
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebKitGTKAssignee: Zan Dobersek <zan>
Status: REOPENED ---    
Severity: Normal CC: achristensen, aperez, bugs-noreply, calvaris, cgarcia, ddkilzer, ews-watchlist, jbicha, jfbastien, lantw44, loic.yhuel, mcatanzaro, ysuzuki, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8209
Bug Depends on:    
Bug Blocks: 185135, 185947    
Attachments:
Description Flags
wip patch
none
WIP
none
WIP
none
WIP
none
Patch
none
Archive of layout-test-results from ews200 for win-future
none
Patch for landing none

Description Carlos Alberto Lopez Perez 2018-05-02 08:15:47 PDT
After 231170: <https://trac.webkit.org/changeset/231170> I switched the Ubuntu GTK bot (still on 16.04) to build with Clang.

I did not upgrade the bot to Ubuntu 18.04 because according to https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy WebKit should remain buildable on Ubuntu 16.04 until 2019-04 (at least with a non default toolchain available on the distro like Clang).

But its failing to build.

I have tested several clang versions that are available on the Ubuntu 16.04 repositories, all failing:

1. clang-3.8, clang-3.9 and clang-4.0 still doesn't support "-std=c++17" , some features are available with "-std=c++1z" but not sure if it has all needed because i'm unable to complete a build with it even after passing that on CXXFLAGS
2. clang-5.0 supports "-std=c++17" but the build is still broken with it

Currently the bot its running with clang-5.0

You can check the build failure here (clang-5.0): https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/11962/steps/compile-webkit/logs/stdio
Comment 1 Adrian Perez 2018-05-02 10:13:24 PDT
JFTR, building with Clang 6.0 is not possible anymore as well.
The error in this case is:

 ../Source/WTF/wtf/Optional.h:224:26: error: explicit specialization of non-template class 'optional'
  template <class T> class optional<T&>;
                           ^       ~~~~

I tried debugging a bit, and the <optional> header does get included
with Clang 6.0, but that is as far as I can go right now. I may look
into this again when I'm less busy.
Comment 2 Carlos Alberto Lopez Perez 2018-05-02 10:22:54 PDT
It looks the failure started to happen when we passed the flag "-std=c++17" to the build.

Trying to build r231169 with this simple patch

--- a/Source/cmake/WebKitCompilerFlags.cmake
+++ b/Source/cmake/WebKitCompilerFlags.cmake
@@ -102,7 +102,7 @@ if (COMPILER_IS_GCC_OR_CLANG)
                                              -Wno-unknown-argument)
     else ()
         WEBKIT_APPEND_GLOBAL_COMPILER_FLAGS(-fno-exceptions)
-        WEBKIT_APPEND_GLOBAL_CXX_FLAGS(-std=c++14
+        WEBKIT_APPEND_GLOBAL_CXX_FLAGS(-std=c++17
                                        -fno-rtti)
 
         if (WIN32)


Makes it already fail (even when no c++17 feature is still used as that was introduced in r231170).

Looking at what gcc and clang define with -std=c++14 vs -std=c++17 I see some differences.

$ touch x.cpp

$ g++-6 -std=c++14 -dM -E x.cpp |nc termbin.com 9999
http://termbin.com/sx9x

$ g++-6 -std=c++17 -dM -E x.cpp |nc termbin.com 9999
http://termbin.com/8ff5

$ clang++-5.0 -std=c++14 -dM -E x.cpp |nc termbin.com 9999
http://termbin.com/6k6p

$ clang++-5.0 -std=c++17 -dM -E x.cpp |nc termbin.com 9999
http://termbin.com/y4jy


 One that rings a bell is For example clang-5.0 with -std=c++17 sets "#define __cplusplus 201703L" meanwhile GCC 6.3 with  -std=c++17 sets "#define __cplusplus 201500L".
Comment 3 Carlos Alberto Lopez Perez 2018-05-02 11:49:44 PDT
Created attachment 339322 [details]
wip patch

This allows for me the GTK build to pass with clang-5.0. No idea if it will break anything else. Let's see what the EWS says.
Comment 4 Adrian Perez 2018-05-02 13:22:37 PDT
(In reply to Carlos Alberto Lopez Perez from comment #3)
> Created attachment 339322 [details]
> wip patch
> 
> This allows for me the GTK build to pass with clang-5.0. No idea if it will
> break anything else. Let's see what the EWS says.

I tried your patch locally with Clang 6.0; but it ends up failing in
a different way (error output below).

---

In file included from ../../Source/WTF/wtf/persistence/PersistentDecoder.cpp:26:
In file included from ../../Source/WTF/config.h:31:
In file included from ../../Source/WTF/wtf/FastMalloc.h:26:
../../Source/WTF/wtf/StdLibExtras.h:554:38: error: reference to 'in_place_t' is ambiguous
__IN_PLACE_INLINE_VARIABLE constexpr in_place_t in_place { };
                                     ^
../../Source/WTF/wtf/StdLibExtras.h:551:8: note: candidate found by name lookup is 'std::in_place_t'
struct in_place_t {
       ^
/usr/bin/../include/c++/v1/utility:904:25: note: candidate found by name lookup is 'std::__1::in_place_t'
struct _LIBCPP_TYPE_VIS in_place_t {
                        ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
Comment 5 JF Bastien 2018-05-02 13:42:38 PDT
You might want to look at the WIP patch here and import some of the workarounds: https://bugs.webkit.org/show_bug.cgi?id=185176
Comment 6 Michael Catanzaro 2018-05-03 17:01:35 PDT
JF, there's an attempt to fix this in bug #185159, but it broke Apple internal builds. It's simply that redefining std::optional is illegal, and WebKit currently does that.

We might need to temporarily revert the switch to C++17 if we can't find a solution quickly.

*** This bug has been marked as a duplicate of bug 185159 ***
Comment 7 Carlos Alberto Lopez Perez 2018-05-08 03:44:26 PDT
(In reply to Michael Catanzaro from comment #6)
> JF, there's an attempt to fix this in bug #185159, but it broke Apple
> internal builds. It's simply that redefining std::optional is illegal, and
> WebKit currently does that.
> 
> We might need to temporarily revert the switch to C++17 if we can't find a
> solution quickly.
> 
> *** This bug has been marked as a duplicate of bug 185159 ***

Build with clang continues to be broken.
I don't think this is a duplicate of 185159. Our Ubuntu LTS bot which tries to build with clang is still broken
Comment 8 Zan Dobersek 2018-05-08 06:55:34 PDT
Created attachment 339812 [details]
WIP
Comment 9 EWS Watchlist 2018-05-08 06:58:01 PDT
Attachment 339812 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Michael Catanzaro 2018-05-08 07:08:35 PDT
Comment on attachment 339812 [details]
WIP

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

> Source/WTF/wtf/Optional.h:219
> +#if defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE)

What on Earth is this testing for...? Unreleased libstdc++?

> Source/WebCore/css/makeSelectorPseudoClassAndCompatibilityElementMap.py:104
> +#define register

Ugh, gperf :(

> Source/cmake/WebKitCompilerFlags.cmake:117
> +        check_cxx_compiler_flag("-std=c++17" CXX_COMPILER_SUPPORTS_CXX17)
> +        if (CXX_COMPILER_SUPPORTS_CXX17)
> +            WEBKIT_APPEND_GLOBAL_CXX_FLAGS(-std=c++17)
> +        else ()
> +            check_cxx_compiler_flag("-std=c++1z" CXX_COMPILER_SUPPORTS_CXX1Z)
> +            if (CXX_COMPILER_SUPPORTS_CXX1Z)
> +                WEBKIT_APPEND_GLOBAL_CXX_FLAGS(-std=c++1z)
> +            else ()
> +                message(FATAL_ERROR "Compiler with C++17 support is required")
> +            endif ()
> +        endif ()

Looks good.
Comment 11 Zan Dobersek 2018-05-08 09:05:43 PDT
Created attachment 339822 [details]
WIP
Comment 12 EWS Watchlist 2018-05-08 09:08:20 PDT
Attachment 339822 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/StdLibExtras.h:587:  in_place_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Zan Dobersek 2018-05-08 11:22:44 PDT
Created attachment 339842 [details]
WIP
Comment 14 EWS Watchlist 2018-05-08 11:25:50 PDT
Attachment 339842 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Carlos Alberto Lopez Perez 2018-05-08 16:23:14 PDT
Comment on attachment 339842 [details]
WIP

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

> Source/WTF/wtf/StdLibExtras.h:546
> +#if __cplusplus < 201703L || (defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE))

Why not just "#if __cplusplus <= 201703L" ?
Comment 16 Michael Catanzaro 2018-05-08 18:35:14 PDT
(In reply to Michael Catanzaro from comment #10)
> Comment on attachment 339812 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339812&action=review
> 
> > Source/WTF/wtf/Optional.h:219
> > +#if defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE)
> 
> What on Earth is this testing for...? Unreleased libstdc++?
Comment 17 Zan Dobersek 2018-05-09 00:36:20 PDT
Comment on attachment 339842 [details]
WIP

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

>> Source/WTF/wtf/StdLibExtras.h:546
>> +#if __cplusplus < 201703L || (defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE))
> 
> Why not just "#if __cplusplus <= 201703L" ?

Because with __cplusplus == 201703L, this clashes with std::in_place_t that's defined in <utility>.
Comment 18 Zan Dobersek 2018-05-09 00:37:27 PDT
(In reply to Michael Catanzaro from comment #16)
> (In reply to Michael Catanzaro from comment #10)
> > Comment on attachment 339812 [details]
> > WIP
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=339812&action=review
> > 
> > > Source/WTF/wtf/Optional.h:219
> > > +#if defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE)
> > 
> > What on Earth is this testing for...? Unreleased libstdc++?

libstdc++ released alongside GCC 6.x, which didn't yet define _GLIBCXX_RELEASE.
Comment 19 Zan Dobersek 2018-05-09 01:37:52 PDT
Created attachment 339946 [details]
Patch
Comment 20 Michael Catanzaro 2018-05-09 08:28:11 PDT
Comment on attachment 339946 [details]
Patch

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

> Source/WTF/wtf/StdLibExtras.h:548
> +// Provide in_place_t when not building with -std=c++17, or when building with libstdc++ 6
> +// (which doesn't define the _GLIBCXX_RELEASE macro that's been introduced in libstdc++ 7).
> +#if __cplusplus < 201703L || (defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE))

Wow, it looks like there's really no better way to do this. Yuck.

https://stackoverflow.com/questions/31506594/how-do-i-test-the-version-of-libstdc-not-gcc-at-compile-time has no good solutions.
Comment 21 JF Bastien 2018-05-09 08:53:30 PDT
(In reply to Zan Dobersek from comment #18)
> (In reply to Michael Catanzaro from comment #16)
> > (In reply to Michael Catanzaro from comment #10)
> > > Comment on attachment 339812 [details]
> > > WIP
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=339812&action=review
> > > 
> > > > Source/WTF/wtf/Optional.h:219
> > > > +#if defined(__GLIBCXX__) && !defined(_GLIBCXX_RELEASE)
> > > 
> > > What on Earth is this testing for...? Unreleased libstdc++?
> 
> libstdc++ released alongside GCC 6.x, which didn't yet define
> _GLIBCXX_RELEASE.

I don't understand why this is needed: GCC 6 isn't supported anymore.
Comment 22 Michael Catanzaro 2018-05-09 09:02:25 PDT
We just dropped support for GCC 5. We still need to support GCC 6.

This bug is for fixing compilation with clang and libstdc++ (in Zan's case, provided by GCC 6). We can't test GCC version, because we're not using GCC. So I think Zan's patch is the best we can do.

C++ 17 is really not going to be as easy as was our transition to C++ 14. :(
Comment 23 JF Bastien 2018-05-09 09:04:30 PDT
(In reply to Michael Catanzaro from comment #22)
> We just dropped support for GCC 5. We still need to support GCC 6.

Oh derp yes, ignore me! :-)
Comment 24 EWS Watchlist 2018-05-09 11:46:45 PDT
Comment on attachment 339946 [details]
Patch

Attachment 339946 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7625241

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html
Comment 25 EWS Watchlist 2018-05-09 11:46:56 PDT
Created attachment 339996 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 26 Carlos Alberto Lopez Perez 2018-05-10 06:10:28 PDT
Comment on attachment 339946 [details]
Patch

Seems it will broke the wincairo build:

C:\WebKit-EWS\WebKit\Source\WebCore\platform\ReferrerPolicy.h(54): error C2039: 'optional': is not a member of 'std'
C:\WebKit-EWS\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/Forward.h(73): note: see declaration of 'std'
C:\WebKit-EWS\WebKit\Source\WebCore\platform\ReferrerPolicy.h(54): error C2143: syntax error: missing ';' before '<'
C:\WebKit-EWS\WebKit\Source\WebCore\platform\ReferrerPolicy.h(54): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
Comment 27 Zan Dobersek 2018-05-14 02:36:19 PDT
Created attachment 340303 [details]
Patch for landing
Comment 28 Zan Dobersek 2018-05-14 05:52:29 PDT
Comment on attachment 340303 [details]
Patch for landing

Clearing flags on attachment: 340303

Committed r231753: <https://trac.webkit.org/changeset/231753>
Comment 29 Zan Dobersek 2018-05-14 05:52:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Carlos Alberto Lopez Perez 2018-05-14 09:51:58 PDT
r231753 fixed the build for the Debian bot which uses clang-3.8.
But the Ubuntu one with clang-5.0 remains broken: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/12353/steps/compile-webkit/logs/stdio

I already tried ordering a clean build. Didn't helped
Comment 31 Zan Dobersek 2018-05-14 10:09:32 PDT
(In reply to Carlos Alberto Lopez Perez from comment #30)
> r231753 fixed the build for the Debian bot which uses clang-3.8.
> But the Ubuntu one with clang-5.0 remains broken:
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/12353/steps/compile-webkit/
> logs/stdio
> 
> I already tried ordering a clean build. Didn't helped

Which libstdc++ is used there?
Comment 32 Carlos Alberto Lopez Perez 2018-05-14 11:25:43 PDT
(In reply to Zan Dobersek from comment #31)
> (In reply to Carlos Alberto Lopez Perez from comment #30)
> > r231753 fixed the build for the Debian bot which uses clang-3.8.
> > But the Ubuntu one with clang-5.0 remains broken:
> > https://build.webkit.org/builders/GTK%20Linux%2064-
> > bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/12353/steps/compile-webkit/
> > logs/stdio
> > 
> > I already tried ordering a clean build. Didn't helped
> 
> Which libstdc++ is used there?

- Debian bot, its currently on 8.0 (jessie), its using libstdc++ 4.9 and clang-3.8 (building fine here after r231753)
- Ubuntu but, its currently on 16.04 (xenial), its using libstdc++ 5.4 and clang-5.0 (still failing to build after r231753)
Comment 33 Michael Catanzaro 2018-05-14 13:43:02 PDT
I think we need to come to some agreement regarding old versions of libstdc++. Our current dependencies policy doesn't mention it specifically, but I think the intent is clear that it's part of the build toolchain and the min version should match GCC....

Surely the bots should be able to static link to libstdc++, right? There is no C++ in the public API, after all, so the build version should not need to match the system version, right?

I know we've discussed this before, but I don't remember the conclusion, and something has gone wrong for us to still be testing libstdc++ provided by GCC 4.9 and 5 after we've increased the compiler requirement past that.
Comment 34 JF Bastien 2018-05-14 13:48:27 PDT
(In reply to Michael Catanzaro from comment #33)
> I think we need to come to some agreement regarding old versions of
> libstdc++. Our current dependencies policy doesn't mention it specifically,
> but I think the intent is clear that it's part of the build toolchain and
> the min version should match GCC....
> 
> Surely the bots should be able to static link to libstdc++, right? There is
> no C++ in the public API, after all, so the build version should not need to
> match the system version, right?
> 
> I know we've discussed this before, but I don't remember the conclusion, and
> something has gone wrong for us to still be testing libstdc++ provided by
> GCC 4.9 and 5 after we've increased the compiler requirement past that.

There's an ABI break between libstdc++4.9 and 5, so you want to compile with  _GLIBCXX_USE_CXX11_ABI=0 and use -Wabi-tag so that you can compile with a version after 5 yet at runtime use libstdc++4.9 and older.
Comment 35 Adrian Perez 2018-05-14 14:18:37 PDT
(In reply to JF Bastien from comment #34)
> (In reply to Michael Catanzaro from comment #33)
> > I think we need to come to some agreement regarding old versions of
> > libstdc++. Our current dependencies policy doesn't mention it specifically,
> > but I think the intent is clear that it's part of the build toolchain and
> > the min version should match GCC....
> > 
> > Surely the bots should be able to static link to libstdc++, right? There is
> > no C++ in the public API, after all, so the build version should not need to
> > match the system version, right?
> > 
> > I know we've discussed this before, but I don't remember the conclusion, and
> > something has gone wrong for us to still be testing libstdc++ provided by
> > GCC 4.9 and 5 after we've increased the compiler requirement past that.
> 
> There's an ABI break between libstdc++4.9 and 5, so you want to compile with
> _GLIBCXX_USE_CXX11_ABI=0 and use -Wabi-tag so that you can compile with a
> version after 5 yet at runtime use libstdc++4.9 and older.

Indeed, there are some cases in which it is desirable to be able to
use the libstdc++ 4.9 ABI at runtime. The main case that comes to mind
is being able to run WebKit{GTK+,WPE} on embedded systems which have
their userland built with GCC <5.0: we can build a newer cross-GCC, and
then use that to build WebKit, which will link against system libraries
which use the old ABI, and use libstdc++ <5.0 at runtime.
Comment 36 Michael Catanzaro 2018-05-14 15:24:21 PDT
I don't think that's even relevant here, because downgrades are never expected to work, right? You can't build against libstdc++ 8 and expect it to work with libstdc++ 5, even with the exact same ABI version, right?

There is another problem. There is zero ABI stability until the language standard is marked as non-experimental. So ABI stability for -std=c++17 won't begin until libstdc++ 9 at the *earliest*.
Comment 37 Carlos Alberto Lopez Perez 2018-05-14 16:00:24 PDT
(In reply to Michael Catanzaro from comment #33)
> I think we need to come to some agreement regarding old versions of
> libstdc++. Our current dependencies policy doesn't mention it specifically,
> but I think the intent is clear that it's part of the build toolchain and
> the min version should match GCC....
> 

On a GNU/Linux distribution libstdc++ is considered a core library. You can't upgrade it without dist-uprading the whole distribution to the next version of the distribution.

AFAIK the only way of keeping WebKit buildable once GCC is not new enough is with Clang, because its not possible to upgrade GCC without updating libstdc++ (which would mean dist-upgrading)

So, either we keep doing this (using Clang on this extra year of support promised by our dependencies policy) or we give up on that extra year and we dist-upgrade the bots to the last version of the distro and use newer GCC.
Comment 38 Michael Catanzaro 2018-05-14 19:06:53 PDT
(In reply to Michael Catanzaro from comment #33)
> There is
> no C++ in the public API, after all, so the build version should not need to
> match the system version, right?

Is that wrong?

Practically speaking, we just need some way we can tell these LTS distros to build WebKit with newer build toolchains, and expect it to work at runtime where the system copy of libstdc++ is older. Static linking to libstdc++ seems like the only way it could be possible. Otherwise, we're going to be stuck supporting ancient versions of libstdc++ and that's going to really seriously limit the benefit to bumping the compiler version....
Comment 39 Carlos Alberto Lopez Perez 2018-05-14 21:28:01 PDT
(In reply to Michael Catanzaro from comment #38)
> (In reply to Michael Catanzaro from comment #33)
> > There is
> > no C++ in the public API, after all, so the build version should not need to
> > match the system version, right?
> 
> Is that wrong?
> 
> Practically speaking, we just need some way we can tell these LTS distros to
> build WebKit with newer build toolchains, and expect it to work at runtime
> where the system copy of libstdc++ is older.
> Static linking to libstdc++ seems like the only way it could be possible. 

I have no idea if this is possible. But I'm certainly sure this is far from practical. How do you fetch a newer libstdc++ and build against it?

Also.. won't this be a problem when webkit links with system libraries implemented in c++ (like icu) that link with the (older) system libstdc++ ?
Comment 40 Loïc Yhuel 2018-05-15 06:21:00 PDT
(In reply to Michael Catanzaro from comment #38)
> Static linking to libstdc++ seems like the only way it could be possible.

Building with clang and libc++ should be possible.
libc++ and libstdc++ can be used in the same process, as long as there isn't any STL object in the API between parts built with the different libraries (or any C++ object if libc++ is built with libc++abi instead of libstdc++(libsupc++)).

But Ubuntu 16.04 has libc++ 3.7 (older than the default clang !), which has issues IIRC (header issues which are fixed later upstream and haven't been backported, and missing headers on install).
Could we ask Ubuntu to update it ? It doesn't impact the whole distribution like libstdc++, only android-sdk-build-tools seems to use it.
Comment 41 Carlos Alberto Lopez Perez 2018-05-15 08:51:30 PDT
(In reply to Loïc Yhuel from comment #40)
> (In reply to Michael Catanzaro from comment #38)
> > Static linking to libstdc++ seems like the only way it could be possible.
> 
> Building with clang and libc++ should be possible.
> libc++ and libstdc++ can be used in the same process, as long as there isn't
> any STL object in the API between parts built with the different libraries
> (or any C++ object if libc++ is built with libc++abi instead of
> libstdc++(libsupc++)).
> 
> But Ubuntu 16.04 has libc++ 3.7 (older than the default clang !), which has
> issues IIRC (header issues which are fixed later upstream and haven't been
> backported, and missing headers on install).
> Could we ask Ubuntu to update it ? It doesn't impact the whole distribution
> like libstdc++, only android-sdk-build-tools seems to use it.

Well, the intention of providing this extra year of support is (IMHO):

 1. To allow developers working in webkit continue using their LTS or Stable distro without being forced to update as soon as a new one is released. I guess we already failed here because WebKit has been broken on Ubuntu 16.04 for weeks already.

 2. To allow distributions that want to keep providing security upgrades to their LTS/stable distributions (which means updating to a major version of WebKitGTK+) to be able to do that.

I'm already not happy with the idea that we have to tell them to switch the compiler (from GCC to Clang) to be able to do that. But the idea that we also are going to tell them to statically build with another c++ library that nobody has tested in GNU/Linux its kind of crazy (IMHO).

There is a clear conflict of interest here between those that want to use the very last standard of C++ (C++-17 which has been released only a year ago) and those that want last WebKit to be buildable on Stable/LTS distributions (which are distributions that were released up to 3 years ago and that don't provide updated toolchains).

Jeremy (CC'ed) ... is Ubuntu planning to keep providing security upgrades for WebKitGTK+ beyond 2.20 (shipping 2.22.x) for Ubuntu LTS 16.04? If not we may just give up on this extra year of support.
Comment 42 Michael Catanzaro 2018-05-15 09:14:16 PDT
(In reply to Carlos Alberto Lopez Perez from comment #39)
> I have no idea if this is possible. But I'm certainly sure this is far from
> practical. How do you fetch a newer libstdc++ and build against it?

The easiest solution would be to build the compiler (including standard library) as part of the WebKit package build. libc++ would work too.

(In reply to Carlos Alberto Lopez Perez from comment #41)
> Jeremy (CC'ed) ... is Ubuntu planning to keep providing security upgrades
> for WebKitGTK+ beyond 2.20 (shipping 2.22.x) for Ubuntu LTS 16.04? If not we
> may just give up on this extra year of support.

Well that's the thing, it looks like they want five years of support, and I think we can provide that. So I want to increase the dependencies policy from three years to five for runtime deps. But we surely cannot do more than two years for the build toolchain.
Comment 43 Loïc Yhuel 2018-05-15 09:48:41 PDT
(In reply to Carlos Alberto Lopez Perez from comment #41)
> I'm already not happy with the idea that we have to tell them to switch the
> compiler (from GCC to Clang) to be able to do that. But the idea that we
> also are going to tell them to statically build with another c++ library
> that nobody has tested in GNU/Linux its kind of crazy (IMHO).
> 
Chromium links statically with its own libc++ (using its own clang) :
https://bugs.chromium.org/p/chromium/issues/detail?id=593874
It seems to be used on Ubuntu, while other distributions may do otherwise :
 - Fedora builds chromium with gcc and libstdc++
 - Archlinux uses system clang and libstdc++
Comment 44 Adrian Perez 2018-05-15 11:38:09 PDT
(In reply to Michael Catanzaro from comment #36)
> I don't think that's even relevant here, because downgrades are never
> expected to work, right? You can't build against libstdc++ 8 and expect it
> to work with libstdc++ 5, even with the exact same ABI version, right?

Yes, it can work: remember that templates are expanded and the code they
generate incorporated in the built binaries. That is what one gets from
headers, while “libstdc++-.so.<num>” contains non-templated functionality.
The ABI version refers to what the latter includes, and by defining 
“_GLIBCXX_USE_CXX11_ABI=0” what we are telling the C++ headers is that
they should use symbols which are available in libstdc++ versions prior
to version 5.x. Some more information:

  https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html
  https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html

(On a different note, here's a fun fact: it is possible to build exactly
the same libstdc++ version with different compile-time options and have
a “libstdc++.so.<num>” as result with an incompatible ABI. More here: https://gcc.gnu.org/onlinedocs/libstdc++/manual/configure.html)

> There is another problem. There is zero ABI stability until the language
> standard is marked as non-experimental. So ABI stability for -std=c++17
> won't begin until libstdc++ 9 at the *earliest*.

Not quite. Again: C++ headers can change from one version to another
of the compiler/libstdc++, and as long as the ABI of “libstdc++*.so”
remains stable, programs built with compiler/headers of different
versions will continue to work as long as they target the same ABI for
the shared library. The following is a quote from one of the pages
linked above:

  “Although the changes [in the ABI] were made for C++11 conformance,
   the choice of ABI to use is independent of the -std option used to
   compile your code, i.e. for a given GCC build the default value
   of the _GLIBCXX_USE_CXX11_ABI macro is the same for all dialects.
   This ensures that the -std does not change the ABI, so that it is
   straightforward to link C++03 and C++11 code together.“

As far as I understand the status quo, there are no plans to change the
ABI again, and the C++17 support
Comment 45 Adrian Perez 2018-05-15 11:43:15 PDT
(In reply to Carlos Alberto Lopez Perez from comment #39)
> (In reply to Michael Catanzaro from comment #38)
> > (In reply to Michael Catanzaro from comment #33)
> > > There is
> > > no C++ in the public API, after all, so the build version should not need to
> > > match the system version, right?
> > 
> > Is that wrong?
> > 
> > Practically speaking, we just need some way we can tell these LTS distros to
> > build WebKit with newer build toolchains, and expect it to work at runtime
> > where the system copy of libstdc++ is older.
> > Static linking to libstdc++ seems like the only way it could be possible. 
> 
> I have no idea if this is possible. But I'm certainly sure this is far from
> practical. How do you fetch a newer libstdc++ and build against it?
> 
> Also.. won't this be a problem when webkit links with system libraries
> implemented in c++ (like icu) that link with the (older) system libstdc++ ?

Yes, that would be indeed a problem. That is also the reason why
distributions in general ship shared libraries for libstdc++. One
should only link libstdc++ statically if the whole program is linked
statically — which is something we would rather not do for WebKit.

...and I suspect that is one of the reasons the “_GLIBCXX_USE_CXX11_ABI=0”
escape hatch was introduced as well.
Comment 46 Adrian Perez 2018-05-15 11:45:13 PDT
Regarding Clang's libc++, I have been using it regularly without
any issue at all. If something, it results in WebKit binaries
which use slightly less memory. Sometimes I feel like I am one
of the few people around building WebKitGTK+ regularly with
Clang (and libc++) ;-)
Comment 47 JF Bastien 2018-05-15 11:48:06 PDT
(In reply to Adrian Perez from comment #46)
> Regarding Clang's libc++, I have been using it regularly without
> any issue at all. If something, it results in WebKit binaries
> which use slightly less memory. Sometimes I feel like I am one
> of the few people around building WebKitGTK+ regularly with
> Clang (and libc++) ;-)

We do too ;-)
Comment 48 Carlos Alberto Lopez Perez 2018-05-17 19:53:39 PDT
(In reply to Carlos Alberto Lopez Perez from comment #32)
> (In reply to Zan Dobersek from comment #31)
> > (In reply to Carlos Alberto Lopez Perez from comment #30)
> > > r231753 fixed the build for the Debian bot which uses clang-3.8.
> > > But the Ubuntu one with clang-5.0 remains broken:
> > > https://build.webkit.org/builders/GTK%20Linux%2064-
> > > bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/12353/steps/compile-webkit/
> > > logs/stdio
> > > 
> > > I already tried ordering a clean build. Didn't helped
> > 
> > Which libstdc++ is used there?
> 
> - Debian bot, its currently on 8.0 (jessie), its using libstdc++ 4.9 and
> clang-3.8 (building fine here after r231753)
> - Ubuntu but, its currently on 16.04 (xenial), its using libstdc++ 5.4 and
> clang-5.0 (still failing to build after r231753)

I switched the Ubuntu bot to use clang-3.8 and now is building fine :)

I tested on it this versions of clang that are available on Ubuntu 16.04:
clang-3.8: builds ok
clang-3.9: builds ok
clang-4.0: fails (different failure than 5.0)
clang-5.0: fails

The default version of clang (the one you get after doing "apt-get install clang") on Ubuntu 16.04 is 3.8

So I guess its ok, as webkit remains buildable there using the default clang version (clang-3.8).

At any rate it still would be interesting to fix the build with the other clang versions. This are the macros enabled for each case:

clang++-3.8 -std=c++17 -dM -E -> error: invalid value 'c++17' in '-std=c++17'
clang++-3.8 -std=c++1z -dM -E -> http://termbin.com/q8sf

clang++-3.9 -std=c++17 -dM -E -> error: invalid value 'c++17' in '-std=c++17'
clang++-3.9 -std=c++1z -dM -E -> http://termbin.com/l4u2

clang++-4.0 -std=c++17 -dM -E -> error: invalid value 'c++17' in '-std=c++17'
clang++-4.0 -std=c++1z -dM -E -> http://termbin.com/4i7f

clang++-5.0 -std=c++17 -dM -E -> http://termbin.com/s54p
clang++-5.0 -std=c++1z -dM -E -> http://termbin.com/2ceb
Comment 49 Carlos Alberto Lopez Perez 2018-05-24 09:17:47 PDT
*** Bug 185947 has been marked as a duplicate of this bug. ***
Comment 50 Carlos Alberto Lopez Perez 2018-05-24 09:18:08 PDT
(In reply to Carlos Alberto Lopez Perez from comment #49)
> *** Bug 185947 has been marked as a duplicate of this bug. ***

Clang 6.0 seems also broken :(
Comment 51 Ting-Wei Lan 2018-05-25 08:21:09 PDT
(In reply to Carlos Alberto Lopez Perez from comment #50)
> (In reply to Carlos Alberto Lopez Perez from comment #49)
> > *** Bug 185947 has been marked as a duplicate of this bug. ***
> 
> Clang 6.0 seems also broken :(

Yes, Clang 6 cannot be used to compile WebKitGTK+ 2.21.2 and the current WebKit trunk, but the problem described in bug 185947 is slightly different from the LTS and outdated C++ library issue discussed here. The Clang 6 test was done on two systems which both have up to date C++ libraries: libc++ 6 on FreeBSD 11.2-BETA2 and libstdc++ 8 on Fedora 28.

Clang 5 works fine with both libc++ 4 on FreeBSD 11.1-RELEASE and libc++ 6 on FreeBSD 11.2-BETA2.
Comment 52 David Kilzer (:ddkilzer) 2018-06-07 09:53:44 PDT
Was this bug going to be used to fix the Clang 6.0+ warnings as well, or should we use Bug 185947 instead?
Comment 53 Michael Catanzaro 2018-06-07 10:35:36 PDT
(In reply to David Kilzer (:ddkilzer) from comment #52)
> Was this bug going to be used to fix the Clang 6.0+ warnings as well or
> should we use Bug 185947 instead?

Use bug #185947 instead. This bug is probably going to be closed if the build failure ever gets fixed.