jdk
jdk copied to clipboard
openjdk
8314488: Compile the JDK as C++17
Compile the JDK as C++17, enabling the use of all C++17 language features
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)
Integration blocker
⚠️ Title mismatch between PR and JBS for issue JDK-8314488
Issue
- JDK-8314488: Compiling the JDK with C++17 (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.
Reviewers
- Martin Doerr (@TheRealMDoerr - Reviewer) 🔄 Re-review required (review applies to 50ffeeea)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14988/head:pull/14988
$ git checkout pull/14988
Update a local copy of the PR:
$ git checkout pull/14988
$ git pull https://git.openjdk.org/jdk.git pull/14988/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14988
View PR using the GUI difftool:
$ git pr show -t 14988
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14988.diff
Using Webrev
:wave: Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@TheShermanTanker The following label will be automatically applied to this pull request:
build
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
The minimum compiler requirements have not been implemented yet, as I wanted to leave their discretion to any reviewers before setting a fixed value in the build system
Webrevs
- 06: Full (9286a964)
- 05: Full - Incremental (50ffeeea)
- 04: Full - Incremental (4f196292)
- 03: Full - Incremental (f1a644e3)
- 02: Full - Incremental (477f6b94)
- 01: Full - Incremental (a1f21bbd)
- 00: Full (09cecd7e)
@TheShermanTanker The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).
I disagree with this change. This should be discussed more broadly before trying to get a PR in. The associated JEP has been closed by Mark. Just a "oh well, then I'm doing it without JEP" is not the right way.
Before agreeing to this, I would like to know what actual changes have been planned, and see them weighted against the cost. The cost, as I have stated before, are reviewer churn, implementation risk, and increased cost of downporting patches to older JDK versions.
Mailing list message from Mario Torre on hotspot-dev:
I agree with Thomas and Mark, I too don't see this change favourably.
This change would affect current build environments and setups, introduce unforeseen bugs and as Thomas mentioned the backporting work itself. Even changes due to JEP 347 did potentially make backporting more difficult, however it standardised on a fixed and broadly available version of gcc, and in this case C++17 would mean being forced to update.
The right place for this sort of changes is a JEP and it needs to be widely discussed with porters and the updates maintainers before even going into mainline. In your case the JEP was rejected, I would accept that because this is an invasive change that may not bring relevant benefits, but at the very least the right thing to do would be to write a better proposal and make a [very] compelling case in the JEP on why we may want to update now, with the prerequisite of a discussion on this proposal and its benefits on the main development list to make sure many more eyes did see this.
Eventually there will be a time where we can and maybe should consider c++17 (or later), I don't think this is now however.
Cheers, Mario
On Thu, Aug 17, 2023 at 8:04?AM Thomas Stuefe <stuefe at openjdk.org> wrote:
-- Mario Torre Manager, Software Engineering, Red Hat OpenJDK, Java Champion https://keybase.io/neugens 9704 A60C B4BE A8B8 0F30 9205 5D7E 4952 3F65 7898 Mastodon: https://mastodon.social/@MarioTorre
Red Hat GmbH, Registered seat: Werner von Siemens Ring 12, D-85630 Grasbrunn, Germany
Commercial register: Amtsgericht Muenchen/Munich, HRB 153243, Managing Directors: Ryan Barnhart, Charles Cachera, Michael O'Neill, Amy Ross
Mailing list message from Andrew Haley on build-dev:
On 8/17/23 10:27, Mario Torre wrote:
The right place for this sort of changes is a JEP
Well, maybe. It depends on how much work is involved.
But, to begin with, it'd be interesting to know if the JDK could be compiled with a C++17 compiler. Then -- if it can't -- submit a PR to make it C++17-clean.
-- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Hi Andrew, majority of the work to remove C++17 breaking code from the JDK is actually already complete, this PR cleans out the last 2 places that cannot pass C++17 (a mismatched throw specifier and a register storage class qualifier), after which the JDK is fully compilable as C++17. I had taken Mark's rejection to mean that going to C++17 is not worth a JEP, unlike JEP-347. Is this still too soon?
Hi Julian,
Given the JEP was closed/rejected as being unnecessary it didn't really make sense to present this as an Implementation of the JEP. This is simply the last couple of fixes to make code C++17 clean and to switch to requiring C++17. The former seems quite reasonable to me. The latter does require further discussion and buy-in.
Thanks
There may be other changes needed either in preparation or as part of making the change to the language version.
-
Dynamic allocation of over-aligned types is supported in C++17. We might need to update our allocation base classes (like CHeapObj<>) to account for this as part of the update. I'm not sure it can be deferred to a separate followup.
-
C++17 exception specifications are part of the type system. "Valid C++14 code may fail to compile or produce different results ..." This needs to be looked at before changing the language selection switch.
-
Dynamic exception specifications were previously deprecated, and are removed by C++17. It looks like some compilers are still permitting the no-throw case, but we might need to convert to using noexcept before changing the language selection switch.
There may be others that I've forgotten or haven't noticed.
This change needs to be motivated. One way to do that would be to file CRs to
change the style guide to permit various features. Label them with cpp17.
Discuss them prior to making the language change, describing why they are
important. A sufficient set of such provides an argument for making the
language change.
This change needs to be motivated. One way to do that would be to file CRs to change the style guide to permit various features. Label them with
cpp17. Discuss them prior to making the language change, describing why they are important. A sufficient set of such provides an argument for making the language change.
Is it impractical to drop the obsolete features of C++11, working in the common subset of C++11 and C++17?
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Requiring xlc17 aka openxl means any AIX rte requirements change as well. Binary compatibility, rte , etc.
We're not changing it for existing releases. I don't think non-LTS releases play a significant role regarding such compatibility. Next LTS is supposed to be JDK 25 (2025-09-16, https://www.java.com/releases/).
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
In case you want to update the required compiler versions as part of this PR: We have tested -TOOLCHAIN_MINIMUM_VERSION_xlc="16.1.0.0011" +TOOLCHAIN_MINIMUM_VERSION_xlc="17.1.1.4" (Already discussed with Kim.)
In conjunction with changing to C++17, I suggest the following changes to the minimum compiler versions, as indicated in make/autoconf/toolchain.m4
old: make/autoconf/toolchain.m4 TOOLCHAIN_MINIMUM_VERSION_clang="3.5" TOOLCHAIN_MINIMUM_VERSION_gcc="6.0" TOOLCHAIN_MINIMUM_VERSION_microsoft="19.28.0.0" # VS2019 16.8, aka MSVC 14.28 TOOLCHAIN_MINIMUM_VERSION_xlc="16.1.0.0011"
proposed new: TOOLCHAIN_MINIMUM_VERSION_clang="13.0" TOOLCHAIN_MINIMUM_VERSION_gcc="9.0" TOOLCHAIN_MINIMUM_VERSION_microsoft="19.28.0.0" # VS2019 16.8, aka MSVC 14.28 TOOLCHAIN_MINIMUM_VERSION_xlc="17.1.1.4"
Here's the rationale for each of these:
gcc: https://gcc.gnu.org/gcc-9/changes.html "The C++17 implementation is no longer experimental."
open xl c++ for aix https://www.ibm.com/docs/en/openxl-c-and-cpp-aix/17.1.1?topic=features-supported-language-levels supports C17 and C++17, with experimental support for C++20 17.1.0 docs explicitly says clang_version is 13.0.0, with the other version macros set accordingly. 17.1.1 just describes the version macros, but doesn't say what their values are. But the VERSION macro description includes "Clang 15.0.0" in the string. Note that there is now a 17.1.2 version, but the aix-ppc porters haven't proposed going that far.
Visual Studio https://learn.microsoft.com/en-gb/cpp/overview/visual-cpp-language-conformance?view=msvc-170 We already require VS2019 16.8, which covers all of C++17 features listed on that page.
clang https://clang.llvm.org/cxx_status.html c++17 - Clang 5
However, there is a critical bug for which we really want a fix. Using [[noreturn]] seems to be buggy and leads to crashes. This has been seen with clang 12. It appears to be fixed with clang 13.0.0 (Xcode 13.0).
There may also be a bug somewhere in the 13.x release series with the handling of noexcept. See discussion in https://bugs.openjdk.org/browse/JDK-8255082.
Oracle is currently using Xcode 14.3.1 (clang 14.0.3), so I think we wouldn't object to something between 13.0.0 and 14.0.3 as the minimum version.
I agree that before throwing this switch, we need to look at some specific issues that might need to be addressed, discuss the benefits, and also the costs.
As was discussed for the change to C++14, there is never a good time to start introducing the use of new language features as far as backporting is concerned, unless one is going to backport the language change too. We didn't do that for C++14, and I don't think we are going to (nor should) do it for C++17 either. But backporting concerns can't be all powerful, as that will forever prevent potentially significant improvements.
I started to make a list of new language features that seem particularly beneficial or otherwise important. I was going to write style guide updates for these, but haven't gotten very far with that yet.
P0035R4: Dynamic memory allocation for over-aligned data P0135R1: Guaranteed copy elision P0145R3: Refining Expression Evaluation Order for Idiomatic C++ P0292R2: constexpr if P0091R3/P0512R0: Template argument deduction for class templates
Here are some others that might be of interest to us. N4268: Allow constant evaluation for all non-type template arguments N3928: Extending static_assert P0118R1: [[fallthrough]] attribute P0189R1: [[nodiscard]] attribute P0212R1: [[maybe_unused]] attribute P0170R1: Wording for constexpr lambda P0283R2: Ignoring unsupported non-standard attributes P0061R1: __has_include for C++17 P0386R2: Inline variables
@kimbarrett
P0035R4: Dynamic memory allocation for over-aligned data
Do we really need this? I ask because, in the end, this will result in something like posix_memalign to be called, and I remember it being notorious for causing large footprint overhead depending on how smart the underlying allocator is about using alignment waste.
It will also be non-trivial to implement in hotspot since NMT uses malloc headers. Barring a rewrite of NMT malloc metadata tracking (e.g. using a hash map, which would be more costly both in terms of performance and, probably, footprint), malloc headers would have to be revised. Probably would need to be dynamic-sized. This is the reason we did not bother wrapping posix_memalign.
@kimbarrett
P0035R4: Dynamic memory allocation for over-aligned data
Do we really need this? I ask because, in the end, this will result in something like
posix_memalignto be called, and I remember it being notorious for causing large footprint overhead depending on how smart the underlying allocator is about using alignment waste.It will also be non-trivial to implement in hotspot since NMT uses malloc headers. Barring a rewrite of NMT malloc metadata tracking (e.g. using a hash map, which would be more costly both in terms of performance and, probably, footprint), malloc headers would have to be revised. Probably would need to be dynamic-sized. This is the reason we did not bother wrapping posix_memalign.
We already have code that (incorrectly) expects dynamic allocation to support overalignment. There are several classes that overalign (often cache align) a member to avoid false sharing, but are dynamically allocated. See most (all?) uses of ZCACHE_ALIGNED for some examples.
One way to fix this would be to give those classes their own operator new to perform aligned allocation somehow. That's what was done for OopStorage::Block, but it's clumsy and likely wasteful of memory. And it's easy to forget. A general solution would probably be better. But yes, NMT malloc headers certainly make the general problem challenging. The approach currently used for OopStorage::Block can be generalized and hooked into the standard mechanism. But maybe there are (possibly non-portable) alternatives that avoid the memory waste?
In case you want to update the required compiler versions as part of this PR: We have tested -TOOLCHAIN_MINIMUM_VERSION_xlc="16.1.0.0011" +TOOLCHAIN_MINIMUM_VERSION_xlc="17.1.1.4" (Already discussed with Kim.)
Also discussed with the aix-ppc port maintainers at IBM.
Is it impractical to drop the obsolete features of C++11, working in the common subset of C++11 and C++17?
I'm not sure what is being suggested. Maybe some examples would help.
Looks basically still good. The only issue I see is requiring clang 14.0 on MacOS is not in sync with "Other JDK 22 build platforms" (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). @MBaesken: Do you know if we can use a newer clang?