Closed Bug 1579189 Opened 3 months ago Closed 5 days ago

bump our minimum supported clang version on Linux (maybe Mac) to something newer

Categories

(Firefox Build System :: General, task)

task
Not set

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: froydnj, Assigned: froydnj, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

My most recent C++17 try run indicated that clang 4.0 doesn't even support -std=gnu++17. Maybe it supports -std=gnu++1z or something like that. Regardless, bumping to a newer clang version to get real C++17 support seems like a reasonable thing to do.

Options:

  1. (the play-it-safe option) Bump to whatever supports -std=gnu++17 or whatever claims support for the latest C++17 draft.
  2. (more aggressive) Bump to clang 8.0 (which is what we support on Windows).

I think we can afford to be slightly more aggressive on our minimum clang version than we are on our minimum GCC version:

  1. Our minimum GCC version needs to be somewhat restrained to make Linux distributions's lives easier.
  2. People compiling with clang are probably using a new-ish version anyway.
  3. If they're not using a new-ish version, we have bootstrappable clang toolchains for them to use.
  4. If they, for some reason, can't use our bootstrappable toolchain...well, they can use GCC (see point 1).

glandium, opinions?

Flags: needinfo?(mh+mozilla)

Looks like clang 5 added gnu++17:

erahm@shetland$ clang-4.0 test.cpp -std=gnu++17
error: invalid value 'gnu++17' in '-std=gnu++17'
erahm@shetland$ clang-5.0 test.cpp -std=gnu++17
erahm@shetland$

Bumping to clang 8 makes sense to me, I think on Windows we're actually going to want to bump to clang 9 as it has fixes for aarch64.

I think clang 5 is technically our minimum supported version on Mac...

Our minimum GCC version needs to be somewhat restrained to make Linux distributions's lives easier.

And so does clang... because here's the catch: we do require clang on GCC builds, for bindgen.

Flags: needinfo?(mh+mozilla)

(In reply to Eric Rahm [:erahm] from comment #1)

Looks like clang 5 added gnu++17:

erahm@shetland$ clang-4.0 test.cpp -std=gnu++17
error: invalid value 'gnu++17' in '-std=gnu++17'
erahm@shetland$ clang-5.0 test.cpp -std=gnu++17
erahm@shetland$

Bumping to clang 8 makes sense to me, I think on Windows we're actually going to want to bump to clang 9 as it has fixes for aarch64.

Windows already requires clang-cl/clang 8.0; I have no issues bumping there because Windows-specific fixes are still coming in.

Can you see what clang-5.0 -dM -E -x c /dev/null -std=gnu++17 -o - |grep __cplus says?

Flags: needinfo?(erahm)

(In reply to Nathan Froyd [:froydnj] from comment #4)

Can you see what clang-5.0 -dM -E -x c /dev/null -std=gnu++17 -o - |grep __cplus says?

#define __cplusplus 201703L

Flags: needinfo?(erahm)

So clang 4, for whatever reason, fails to compile things with -std=gnu++1z:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266385391&revision=58f7609048db64e850b3a35457ef009790c06d32

clang is failing to find this overload:

https://searchfox.org/mozilla-central/source/dom/crypto/CryptoBuffer.h#38-43

because it's failing to consider the base classes of the object passed in (I think).

That sort of bug is not something I'm really interested in diving into.

There's also the interesting problem that clang 4 built with GCC 7 has...issues with libstdc++ 7.x headers, due to incomplete C++17 support. So we'd have to build clang 4 with GCC 6...which doesn't really help with trying to provide C++17-compatible headers during the build process.

Looking at our Linux compatibility matrix:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Linux_compatibility_matrix

The only interesting distros that have clang < 5 and are still under active support are:

So the only sticking point would be Debian 8, which doesn't even have ESR 68, much less whatever our next ESR (~75?) would be.

So I don't think bumping our minimum clang version would have much impact on Linux distributions. I also think developers' lives would be much easier not having to contend with a compiler with a creaky implementation of C++17 features, if any actually exist at all.

Would you be OK with bumping things to clang 5?

Flags: needinfo?(mh+mozilla)

So the only sticking point would be Debian 8, which doesn't even have ESR 68

It doesn't have it yet, because 60 is still supported. But it will have it soon. OTOH, by the time ESR68 is EOLed, Debian 8 will have been EOLed too.
Note, though, that availability in backports doesn't tell much about stable (wrt Debian 9). However, rust updates will require llvm updates, so clang backports are not much of a stretch. But that's only me talking, that would need more feedback from the involved parties.

NI Sylvestre for clang in Debian, Wolfgang for SLES and Martin for RedHat.

Flags: needinfo?(stransky)
Flags: needinfo?(sledru)
Flags: needinfo?(mozilla)
Flags: needinfo?(mh+mozilla)

And Chris for Ubuntu.

Flags: needinfo?(chrisccoulson)

For openSUSE raising the requirement is not an issue.
For SLES12 I'm guessing that it can be solved but pointed Martin here to validate.

Flags: needinfo?(mozilla)

Apparently it would be really nice for C++17 support if we could go all the way to clang 7, as clang 6 still has some C++17 bugs. So the farther we can push clang support, the better.

RedHat/Fedora uses gcc everywhere to build Firefox and we recently use gcc8/clang7 from additional developer repos to build Firefox on all RHELs. Also we can easily update any compiler/toolchain packages by Red Hat compiler collections so I'm fine with the change as it does not impact us.

Flags: needinfo?(stransky)

SUSE has clang 7 available as a build-dependency on SLE-12 and SLE-15. We might manage to get clang 8 for the next ESR-release, but staying at <= 7 would probably be better for us.

About Debian Jessie (aka 8), there are a bunch of newer versions of clang available here: http://apt.llvm.org/jessie/pool/main/l/ Back in April, I stopped maintaining it for Jessie because Jessie backports aren't available anymore https://lists.debian.org/debian-backports-announce/2018/07/msg00000.html and nobody complained.
Porting more recent version of Rust on Jessie might be also a bunch of work.

So, I recommend we don't support Debian 8 anymore (after 68 is EOL).

For Ubuntu, as the clang packages are the same as apt.llvm.org, we could take care of uploading a new version of clang into xenial-updates.
But I don't know about Rust backporting. I guess Rust upstream have some CI on these LTS version.

Flags: needinfo?(sledru)

I pinged Chris over email last month and I pinged him again today. I think it's fair to say that if we don't get a response by the end of the week, we can assume, given Debian's OK with this, that Ubuntu is too?

We need this for "full" C++17 support (everything is supported, but some
C++17 features still have bugs) and this change also brings Linux into
parity with out Mac requirements.

Depends on: 1593703
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64119136e15
raise the minimum clang version to 5; r=#build
Status: NEW → RESOLVED
Closed: 5 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.