1325632 - (C++14) compile with a C++14-compatible mode

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

Now that we require GCC 4.9, we can do this and reap (some of) the benefits of C++14.

Comment 1

[Nathan Froyd [:froydnj]]

FWIW, clang does not enable C++14 sized deallocation unless -fsized-deallocation is provided, GCC doesn't support it until 5.x, and we already pass the disabling flag on MSVC (bug 1160146).  So we don't have to worry about that...

Comment 2

[Nathan Froyd [:froydnj]]

The obvious patch works just fine locally and on try.

The problems are in the details: GCC 4.9 and GCC 5.x report different versions for __cplusplus with -std=gnu++14, so we need to handle both of those.  The tests, however, assume that you can pass -std=gnu++14 to GCC, but neither GCC 4.7 or GCC 4.8 support that option (4.8 supports -std=gnu++1y, which is just another name for -std=gnu++11).  (-std=gnu++14 is the first flag we've added with this property.)  The upshot is that the tests for GCC 4.7 ignore -std=gnu++14, and we wind up hitting this error:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#696-698

rather than the friendlier error:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#705-708

I don't think reordering those checks is correct.  I don't think changing the tests to use the error message from the first check is correct, and I don't think changing the tests to lie and say that -std=gnu++14 is supported by GCC 4.7 is correct, either.  So I'm not sure where to go from there.  WDYT?

Comment 3

[Nathan Froyd [:froydnj]]

I guess we could just add a version check in toolchain.configure as to whether we could add -std=gnu++14?

Comment 4

[Mike Hommey [:glandium]]

I don't know what you tried, but there should be no problem, because what should happen is that g++ is run once without flags, which returns its version and the C++ version it supports. Then, if the version is too old (< 4.9), we error out. Next, if the C++ version doesn't match what we want, we try again with -std=gnu++14, which will give us the right version. That can easily be double checked in test_toolchain_configure.py too (in fact, it should be tested in test_toolchain_configure.py)

Note that it's easier now that we don't support < 4.9, but it would still be possible to fit -std=gnu++1y vs -std=gnu++14 in there if that were necessary.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #4)
> I don't know what you tried, but there should be no problem, because what
> should happen is that g++ is run once without flags, which returns its
> version and the C++ version it supports. Then, if the version is too old (<
> 4.9), we error out. Next, if the C++ version doesn't match what we want, we
> try again with -std=gnu++14, which will give us the right version. That can
> easily be double checked in test_toolchain_configure.py too (in fact, it
> should be tested in test_toolchain_configure.py)

Yes, I understand that's what's tested in test_toolchain_configure.py; that's what I'm trying to fix. :)

What you describe is not what actually happens: we check the compiler here:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#662-663

and then run a separate check in case we received more flags:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#665-670

i.e. something like -std=gnu++14.  Only then do we start checking whether the information we received makes any sense.  And, in particular, we check whether the second invocation required any additional flags:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#696-698

*before* we check the GCC version:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#700-708

I guess you're suggesting that the GCC version check be moved up before the second check_compiler call?  Can't wait to watch a bunch of tests break. ;)

> Note that it's easier now that we don't support < 4.9, but it would still be
> possible to fit -std=gnu++1y vs -std=gnu++14 in there if that were necessary.

See comment 2 about command-line support for both of those flags.

Comment 6

[Mike Hommey [:glandium]]

Oh, right. I guess what would make sense is to ignore exceptions from the second call to check_compiler, because the results from the first call will fail the subsequent checks already with a sensible error. And moving the check about info.flags not being empty after the version checks.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move the version checks for GCC and MSVC

toolchain.configure checks for information about the compilers we're
using and accumulates additional flags that we might need to
pass (e.g. switches for C/C++ versions, proper compiler targets, etc.),
and then rechecks the compilers with those additional flags to verify
that those flags are sufficient to configure the compiler properly.
Only after we've checked for the proper flags do we move on to verifying
the compiler versions are sufficient.

However, for GCC and MSVC, we have enough information to verify the
version after the first check.  Doing the version check first simplifies
some edge cases in testing, such as when the compiler version that we're
checking doesn't necessarily support options we want to pass to it, and
also makes some tests slightly faster.

We do not move the check for clang versions to the same place, as
checking for the clang version is a complicated business (see the
comments in toolchain.configure).  We do, however, move the clang
version check before the check for whether we needed a second round of
modifications to the flags we pass, since the ordering of those checks
makes slightly more sense.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 2 - compile with C++14 support

So easy!

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Exciting stuff! Will it make it into 53? :-)

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Gerald Squelart [:gerald] from comment #9)
> Exciting stuff! Will it make it into 53? :-)

I think 53 might have branched already?  Or is that only for release, and the actual switchover for the other trees is next week?

Mostly depends on glandium's review at this point. :)

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8827519 [details] [diff] [review]
part 1 - move the version checks for GCC and MSVC

Review of attachment 8827519 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not fan of the fact that version checks are now split in two places. I'd very much prefer the approach from comment 6.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8827520 [details] [diff] [review]
part 2 - compile with C++14 support

Review of attachment 8827520 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +370,5 @@
> +            append_flag('-std=gnu++14')
> +        # GCC 4.9 indicates that it implements draft C++14 features
> +        # instead of the full language.
> +        elif info.type == 'gcc' \
> +             and not (info.language_version == draft_cxx14_version \

and info.language_version not in (cxx14_version, draft_cxx14_version)

Comment 13

[Nathan Froyd [:froydnj]]

Sorry, I saw "move version checks" and completely ignored the exception handling bit.  I'll fix that up.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 1 - modify re-checking the compiler with flags

Updated patch.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8828080 [details] [diff] [review]
part 1 - modify re-checking the compiler with flags

Review of attachment 8828080 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +671,5 @@
> +            if info.flags:
> +                flags += info.flags
> +                info = check_compiler(wrapper + [compiler] + flags, language,
> +                                      host_or_target)
> +        except:

except FatalCheckError:

Comment 16

[Nathan Froyd [:froydnj]]

Sigh.  I'm not sure what try push I was looking at before, but pushing these patches to try reveals the following problems:

* Our clang packages embed the libstdc++ of whatever gcc version they were built with.  So our clang 3.8 packages, being built with gcc 4.8.x, have libstdc++ 4.8.x, which doesn't work so well with C++14.  This is easily fixable, probably by using the packages being built in bug 1331012.  Maybe we should add some automation-only check for this somewhere?

* Android's libc++ does not work with gcc 4.9 and C++14.  Chromium discovered this too:

https://bugs.chromium.org/p/chromium/issues/detail?id=554839

and the next-generation rangev3 library had similar reports filed against it:

https://github.com/ericniebler/range-v3/issues/210

Comments there suggest that gcc >= 5.2 is really needed to make C++14 support work well...or at least work to the point that you can compile libc++ with it.

We have a long-standing issue open with trying to get Android to compile with clang, but it needs work in moz.configure (clang requires a slightly different setup for its sysroot etc. files), and it needs work on the clang side, because clang bloats libxul by ~10% on Android compared to gcc.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> * Android's libc++ does not work with gcc 4.9 and C++14.

One possibility is to switch android to using libstdc++, which should/would be guaranteed to work with GCC 4.9 and C++14.

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #16)
> > * Android's libc++ does not work with gcc 4.9 and C++14.
> 
> One possibility is to switch android to using libstdc++, which should/would
> be guaranteed to work with GCC 4.9 and C++14.

Well, it would if Android's libc wasn't terrible.  The libstdc++ as distributed with NDK r11c turns off C99 support in libstdc++, which disables a host of interesting features.  The problems I ran into were with string formatting (std::to_string isn't provided).  I think we could work around those by providing polyfills, but I'm sure that would make some people pretty unhappy.

Comment 19

[Nathan Froyd [:froydnj]]

We'd also have to hack around lack of support for various std:: math functions, all of which I think we can actually access in C.  Android's libc++ certainly does, but it'd probably be a bridge too far to attempt and shoehorn that support into libstdc++.

Comment 20

[Mike Hommey [:glandium]]

IIRC libstdc++ is large too, and would make the APK larger.

Comment 21

[Chris Peterson [:cpeterson]]

When you enable C++14 support, you will probably want to remove the -Wc++14-compat warning flags from build/moz.configure/warnings.configure.

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

So... the question is, when can we get C++14 support enabled? Is it expected to happen soonish?

I ask because if it can land soon, I can probably use some of the new features which would make some patches I'm currently working on much better. But if it cannot, I'll need to workaround somehow.

Or probably I can actually use some new language features given that the c++14-compat warning is disabled?

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #22)
> So... the question is, when can we get C++14 support enabled? Is it expected
> to happen soonish?
> 
> I ask because if it can land soon, I can probably use some of the new
> features which would make some patches I'm currently working on much better.
> But if it cannot, I'll need to workaround somehow.

This work is blocked by being able to use Clang for our android builds.  And that's blocked on being able to make changes in how we call the compiler on Android, and Clang being terrible for code size on Android.  The changes in how we call the compiler on Android are possibly doable by the end of the month, but the code size badness is out of our control.

> Or probably I can actually use some new language features given that the
> c++14-compat warning is disabled?

You can probably use them inadvertently on MSVC, but it'd be a toss-up as to whether those changes would compile on GCC and Clang.

Comment 24

[(no longer active)]

What is the current status of this bug?

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #24)
> What is the current status of this bug?

Comment 23 is still applicable.  For why clang on android is blocked, see bug 1163171 comment 7.  Recent comments on that bug indicate that compiling with clang has gotten easier, and hearsay from other places on the 'net indicate that clang actually has *two* settings for size optimization, with one being much better than the other.  Those are on my list of things to try soonish.

Why do you ask?

Comment 26

[(no longer active)]

I would like to see if we can adopt the hash table implementation here: https://probablydance.com/2017/02/26/i-wrote-the-fastest-hashtable/

But if we can't borrow the code directly that's OK too, borrowing the ideas is possible as well!  I'd like to know which direction to go.  It would be nice to address the issue of our hashtable slowness to some extent before 57.

Comment 27

[Mike Hommey [:glandium]]

OT for this bug, but answering comment 26: maybe it would make sense to wrap rust's hashtables.

Comment 28

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #27)
> OT for this bug, but answering comment 26: maybe it would make sense to wrap
> rust's hashtables.

I chatted with mystor about this over lunch today, unfortunately we'd lose inlining currently if we did that.  We should be able to borrow ideas though.  I'll for now look for the borrowing ideas approach not code.  Thanks and sorry for derailing the bug!

Comment 29

[Nathan Froyd [:froydnj]]

tjr needs pieces of this bug in bug 1392643, so I'm going to be landing some of those pieces, but *not* the ones that turn on C++14 support.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac9cbe03c9d
part 1 - modify re-checking the compiler with flags; r=glandium

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6ac9cbe03c9d

Comment 32

[Nathan Froyd [:froydnj]]

Attachment: part 3 - don't warn about c++98/c++11 compatibility problems

We're compiling with C++14 and we don't plan to go back.

Comment 33

[Nathan Froyd [:froydnj]]

Attachment: part 4 - compile rust bindings with -fno-sized-deallocation

This probably doesn't make a huge difference, as we're not generating
any code here, but better safe than sorry.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: part 5 - ensure that we compile with -fno-sized-deallocation when possible

We currently turn off the C++14 sized-deallocation facility on MSVC, and
we'd like to ensure we do the same thing for clang and gcc.  To do so,
we add new functionality to moz.configure for checking and adding
compilation flags, similar to the facility for checking and adding
warning flags.  The newly added facility is then used to add
-fno-sized-deallocation to the compilation flags, when the option is
supported.

I don't think it's worth it, currently, to try and deduplicate the warnings and
the compile flags mechanisms.  Interested to hear other people's thoughts.

Comment 35

[Nathan Froyd [:froydnj]]

Turns out we need to remove the sized deallocation overrides we provide:

https://hg.mozilla.org/try/rev/eca3a9794b97ea3bb9719061898415fb45807c4b

I think we can safely remove them, as the warnings that motivated bug 1406197 will warn us if we happen to turn off sized deallocation.

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8925628 [details] [diff] [review]
part 5 - ensure that we compile with -fno-sized-deallocation when possible

Review of attachment 8925628 [details] [diff] [review]:
-----------------------------------------------------------------

I do think it's work it to de-duplicate this and the warnings mechanism, but if that's more than you're interested in doing right now I think it would be perfectly fine to just stick it in compiler-opts.m4 for now.

Comment 37

[Nathan Froyd [:froydnj]]

Attachment: part 5 - split out framework for testing flags

We have code to test whether particular flags are supported for the
compiler we're using.  Unfortunately, that code is tied up with checking
for warning flags.  We're about to add a separate facility for generic
compilation flags, and we'd like to avoid cutting and pasting code if
possible.  Let's split the core code out into a separate, reusable function.

Comment 38

[Nathan Froyd [:froydnj]]

Attachment: part 6 - ensure that we compile with -fno-sized-deallocation when possible

We currently turn off the C++14 sized-deallocation facility on MSVC, and
we'd like to ensure we do the same thing for clang and gcc.  To do so,
we add new functionality to moz.configure for checking and adding
compilation flags, similar to the facility for checking and adding
warning flags.  The newly added facility is then used to add
-fno-sized-deallocation to the compilation flags, when the option is
supported.

Once we do this, we can't define the sized deallocation functions in
mozalloc.h; the compiler will complain that we are using
-fno-sized-deallocation, yet defining these special functions that we'll
never use.  These functions were added for MinGW, where we needed to
compile with C++14 ahead of other platforms to be compatible with MSVC
headers.  But they're no longer necessary, though they would be if we
removed -fno-sized-deallocation; the compiler will complain if we do
that and we'll add them back at that point.

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8927436 [details] [diff] [review]
part 5 - split out framework for testing flags

Review of attachment 8927436 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/compile-checks.configure
@@ +81,5 @@
> +# Determine whether to add a given flag to the given lists of flags for C or
> +# C++ compilation.
> +# - `flag` is the flag to test
> +# - `cflags` is a @dependable for the list of C compiler flags to add to
> +# - `cxxflags` is a @dependable for the list of C++ compiler flags to add to

@dependable isn't typically used as a noun. "@depends function" is. Or just call it a list.

@@ +84,5 @@
> +# - `cflags` is a @dependable for the list of C compiler flags to add to
> +# - `cxxflags` is a @dependable for the list of C++ compiler flags to add to
> +# - `actual_flags` is a list of flags to pass to the compiler instead of merely
> +#   passing `flag`. This is especially useful for checking warning flags. If
> +#   this list is empty, `flag` will be passed on its own.

Call this `test_flags`?

Comment 40

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8927438 [details] [diff] [review]
part 6 - ensure that we compile with -fno-sized-deallocation when possible

Review of attachment 8927438 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for doing this.

Comment 41

[Nathan Froyd [:froydnj]]

Thanks for the review, these are good suggestions.

(In reply to Chris Manchester (:chmanchester) from comment #39)
> ::: build/moz.configure/compile-checks.configure
> @@ +81,5 @@
> > +# Determine whether to add a given flag to the given lists of flags for C or
> > +# C++ compilation.
> > +# - `flag` is the flag to test
> > +# - `cflags` is a @dependable for the list of C compiler flags to add to
> > +# - `cxxflags` is a @dependable for the list of C++ compiler flags to add to
> 
> @dependable isn't typically used as a noun. "@depends function" is. Or just
> call it a list.

"@depends function" works for me.  I don't think you could actually pass a list here?  I confess to not fully understanding how we pass @depends functions and we wind up accumulating flags into them...

> @@ +84,5 @@
> > +# - `cflags` is a @dependable for the list of C compiler flags to add to
> > +# - `cxxflags` is a @dependable for the list of C++ compiler flags to add to
> > +# - `actual_flags` is a list of flags to pass to the compiler instead of merely
> > +#   passing `flag`. This is especially useful for checking warning flags. If
> > +#   this list is empty, `flag` will be passed on its own.
> 
> Call this `test_flags`?

This is much better, thank you.

Comment 42

[Nathan Froyd [:froydnj]]

I'm going to reorder the patches for landing, putting the C++14 activation one (part 2) last, so that we might stand a chance of bisecting through these.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7452d18793ab
part 2 - don't warn about c++98/c++11 compatibility problems; r=ted.mielczarek
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7756d08443d
part 3 - compile rust bindings with -fno-sized-deallocation; r=ted.mielczarek
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeaf5db9a9e2
part 4 - split out framework for testing flags; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/402cad93aa6e
part 5 - ensure that we compile with -fno-sized-deallocation when possible; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a8db01f25a
part 6 - compile with C++14 support; r=glandium

Comment 44

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7452d18793ab
https://hg.mozilla.org/mozilla-central/rev/c7756d08443d
https://hg.mozilla.org/mozilla-central/rev/aeaf5db9a9e2
https://hg.mozilla.org/mozilla-central/rev/402cad93aa6e
https://hg.mozilla.org/mozilla-central/rev/b6a8db01f25a

Comment 45

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Yeah! Thanks for working on this.

Now, for some value of "fun", I tried compiling my long-pending patch (which requires C++ 14) for bug 1329385, and I got:
> gmake[4]: Entering directory '/Users/gerald/Documents/mozilla/obj-mozilla-central/dom/media/gmp'
> In file included from obj-mozilla-central/dom/media/gmp/Unified_cpp_dom_media_gmp1.cpp:38:
> dom/media/gmp/GMPServiceParent.cpp:378:45: error: initialized lambda captures are incompatible with C++ standards before C++14 [-Werror,-Wc++98-c++11-compat]
>     [self, tags, api, nodeIdString, helper, holder(Move(holder))]
>                                             ^
> 1 error generated.

Is that still expected?

Here's the compiler invocation:
> /usr/bin/clang++ -std=gnu++14 -o Unified_cpp_dom_media_gmp1.o -c -fvisibility=hidden -fvisibility-inlines-hidden
>   -DDEBUG=1 -DGMP_SAFE_SHMEM -DOS_POSIX=1 -DOS_MACOSX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE
>   -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I[...] -fPIC -DMOZILLA_CLIENT -include obj-mozilla-central/mozilla-config.h
>   -Qunused-arguments -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers
>   -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return
>   -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++14-compat -Wc++14-compat-pedantic
>   -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion
>   -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat
>   -Wno-gnu-zero-variadic-macro-arguments -Wformat-security -Wno-unknown-warning-option -Wno-return-type-c-linkage
>   -fno-sized-deallocation -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti
>   -fno-exceptions -fno-math-errno -pthread -pipe -g -fno-omit-frame-pointer -Werror -Wno-error=shadow  -MD -MP -MF
>   .deps/Unified_cpp_dom_media_gmp1.o.pp  -fcolor-diagnostics
>   obj-mozilla-central/dom/media/gmp/Unified_cpp_dom_media_gmp1.cpp
Mac 10.12.6, Apple LLVM version 8.1.0 (clang-802.0.42)

Comment 46

[Nathan Froyd [:froydnj]]

(In reply to Gerald Squelart [:gerald] from comment #45)
> Yeah! Thanks for working on this.
> 
> Now, for some value of "fun", I tried compiling my long-pending patch (which
> requires C++ 14) for bug 1329385, and I got:
> > gmake[4]: Entering directory '/Users/gerald/Documents/mozilla/obj-mozilla-central/dom/media/gmp'
> > In file included from obj-mozilla-central/dom/media/gmp/Unified_cpp_dom_media_gmp1.cpp:38:
> > dom/media/gmp/GMPServiceParent.cpp:378:45: error: initialized lambda captures are incompatible with C++ standards before C++14 [-Werror,-Wc++98-c++11-compat]
> >     [self, tags, api, nodeIdString, helper, holder(Move(holder))]
> >                                             ^
> > 1 error generated.
> 
> Is that still expected?

That's not expected...but I think we should have removed -Wc++-11-compat from the warnings flags as well.  Open a followup bug?