Skip to content

Upgrade to C++17#2709

Closed
kripken wants to merge 2 commits intomainfrom
c++17
Closed

Upgrade to C++17#2709
kripken wants to merge 2 commits intomainfrom
c++17

Conversation

@kripken
Copy link
Member

@kripken kripken commented Mar 24, 2020

I remember we had to back off this before, but am not sure why. CI seems ok here. Perhaps it was chromium CI that failed?

@kripken kripken requested a review from tlively March 24, 2020 23:30
@tlively
Copy link
Member

tlively commented Mar 25, 2020

SGTM! The README, autoupdate_tests.py, and check.py should also be updated to reflect this change.

@kripken
Copy link
Member Author

kripken commented Mar 28, 2020

cc @dschuff @sbc100 I remember we tried this before but had to revert, but I don't remember why. Maybe lack of c++17 on the clang on chromium CI or something like that?

@dschuff
Copy link
Member

dschuff commented Mar 28, 2020

looks like it was #2358
I think Chromium's clang should be able to handle it. Looks liek the issue was debian stable?

@kripken
Copy link
Member Author

kripken commented Mar 28, 2020

Oh interesting, thanks @dschuff

Ubuntu LTS will be updated April 23, so less than a month from now. We may want to wait for that. I'm not sure about others like Debian.

What about the CMake on the waterfall that's mentioned there, have we upgraded it to a version that supports c++17, do you know?

@dschuff
Copy link
Member

dschuff commented Mar 28, 2020

We updated that to 3.15 after #2358 so it should be good.

CMakeLists.txt Outdated
set(CMAKE_THREAD_PREFER_PTHREAD ON)
find_package(Threads REQUIRED)
add_cxx_flag("-std=c++14")
add_cxx_flag("-std=c++17")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a builtin CMake command to set the C++ standard?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth doing that transition separately as there be a history as to why are not currently using that.

@tlively
Copy link
Member

tlively commented Apr 8, 2020

@kripken it would be good to push on this so we can start using std::optional and friends.

@kripken
Copy link
Member Author

kripken commented Apr 8, 2020

I was thinking about waiting for Ubuntu LTS to update to a version that works with us, for people building by themselves. That's in 2 weeks.

Although I guess we can do this and people in the interim can use a slightly older binaryen (and emscripten)?

@sbc100
Copy link
Member

sbc100 commented Apr 8, 2020

I yes, this will effect all emsdk users who build from source.. and there are some of those who build from source only because our prebuilt binaries use too modern a version of libc/libc++.

So I guess it would be a shame if binaryen became only reason that all emsdk-from-source users suddenly need c++17.

@sbc100
Copy link
Member

sbc100 commented Apr 8, 2020

Maybe we should take a queue from llvm itself and not require anything more modern than they do?

@sbc100
Copy link
Member

sbc100 commented Apr 16, 2020

Shall we close this and revisit in 6 months?

@kripken
Copy link
Member Author

kripken commented Apr 16, 2020

I'm ok to close this and reconsider later. But about:

I yes, this will effect all emsdk users who build from source.. and there are some of those who build from source only because our prebuilt binaries use too modern a version of libc/libc++.

Do you have an idea who those users are? Do they not have a c++17-enabled compiler?

@sbc100
Copy link
Member

sbc100 commented Apr 16, 2020

Do you have an idea who those users are? Do they not have a c++17-enabled compiler?

They are mostly users to very old linux distributions for whom our prebuilt binaries are too modern. So it seem likely their compilers will be too old. I don't remember off the top of my head exactly which older distros they are using. For that we would need to look back through the old emsdk/emscripten bugs.

@tlively
Copy link
Member

tlively commented Apr 16, 2020

Should we come up with a policy about compiler versions we support that we can apply consistently in the future? Something like "default cmake and compiler for last 2 versions of Ubuntu LTS" or "default cmake and packaged compiler for last version of Ubuntu LTS" or something like that?

@sbc100
Copy link
Member

sbc100 commented Apr 16, 2020

As I say I don't think binaryen's policy should be more restrictive than that llvm, otherwise binaryen itself becomes the limiting factor on the portability of the whole of emsdk.

Would a policy of just deferring to llvm's policy be objectionable?

@tlively
Copy link
Member

tlively commented Apr 16, 2020

Yeah that sounds fine 👍 Of course LLVM has the same problem and the discussions are much much longer, but that shouldn't actually matter for us here.

@kripken
Copy link
Member Author

kripken commented Apr 16, 2020

Following LLVM sounds ok to me.

What's the status there? Best I can find is this which is 2 years ago and says they hoped to switch 1 year ago... have they?

@kripken
Copy link
Member Author

kripken commented Apr 24, 2020

I tried to find out more about LLVM's policy on this and found

http://llvm.org/docs/DeveloperPolicy.html#toolchain

A mailing list discussion for c++14 is here: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131486.html

The main things there are:

  • That the LLVM code itself can be compiled with a 3 year old toolchain.
  • That they have consensus on the mailing list (to see if people are using toolchains that couldn't compile LLVM).

In general it seems reasonable for us to follow them, but it is a lengthy process there with far more stakeholders than we have.

In other news Ubuntu 20.04 LTS has been released, so the latest version there can compile c++17.

@tlively
Copy link
Member

tlively commented Apr 24, 2020

Yeah I guess I don't really want to have to wait for the LLVM community to reach consensus and then make another release. Following latest LTS Ubuntu sounds reasonable? I don't know. Who are our actual customers we are trying to support with old compilers?

@dschuff
Copy link
Member

dschuff commented Apr 27, 2020

The "latest" LTS isn't really relevant though; the whole point of an LTS is that you don't have to upgrade to the latest one when it comes out.

@kripken
Copy link
Member Author

kripken commented Apr 27, 2020

I agree it's a fair point that it's safer to follow LLVM, and LTS users may wait almost the entire 5 years before upgrading, yeah.

I'd like to re-propose my idea to work around this issue with wasm or wasm2c builds: if a user tells us "Binaryen can't compile on my system, it uses c++17/20/whatever" then we can either

  • Give them a wasm2c build - a single big C file. They can definitely compile that.
  • Give them a nodejs build, i.e., JS+wasm build of Binaryen. Then they don't even need to compile, assuming they have node.js. Such a build would be slower (usual minor wasm overhead, plus lack of threads/SIMD/etc. as it's not a new node.js presumably) but could still be usable on non-huge projects.

It would take some work for us to provide these builds, but not a large amount. since we've already done almost all of it.

The benefit to this is that it decouples the developer requirements from the user requirements: developers do need to actually build Binaryen, but they tend to have newer compilers anyhow (and they want to use new language features!). Users just want to run a build, and only compile if they have to. We have the technology to help them avoid that 😀

@sbc100
Copy link
Member

sbc100 commented Apr 27, 2020

Given that we mostly just want variant types and optional, why not just import llvm::Optional?

Wait ... we already have third_party/llvm-project/include/llvm/ADT/Optional.h as part of binaryen!

@kripken
Copy link
Member Author

kripken commented Apr 27, 2020

Yeah, we do have Optional.h already thanks to the DWARF import from LLVM, fair enough 😃

But c++17 has a lot of pleasant things: https://en.wikipedia.org/wiki/C%2B%2B17#Language Off the top of my head, I'd be happy to use several of those, especially structured bindings declaration (aka auto [a, b] = returnsTuple();)

@dschuff
Copy link
Member

dschuff commented Apr 27, 2020

Having said that, I don't think we necessarily need to follow LLVM; they have more constraints than we do (e.g. bootstrapping OSes and such). for example, I wouldn't be too sad if we required a user to install an upgraded gcc or clang if it were available from their distro (even if it wasn't the default system compiler). It wouldn't be unreasonable to run ahead a bit of LLVM in cases like that.

Also, i don't mind copying/polyfilling std library functionality either.

@dcodeIO dcodeIO mentioned this pull request Sep 5, 2020
Base automatically changed from master to main January 19, 2021 21:59
@tlively
Copy link
Member

tlively commented Nov 29, 2021

We have successfully upgraded to C++17 in the meantime.

@tlively tlively closed this Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants