8314488: Compile the JDK as C++17 #14988

TheShermanTanker · 2023-07-24T01:41:16Z

Compile the JDK as C++17, enabling the use of all C++17 language features

Progress

Change must not contain extraneous whitespace
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

Link to Webrev Comment

bridgekeeper · 2023-07-24T01:42:06Z

👋 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.

openjdk · 2023-07-24T01:43:22Z

@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.

TheShermanTanker · 2023-07-24T03:03:26Z

/label hotspot

openjdk · 2023-07-24T03:04:00Z

@TheShermanTanker
The hotspot label was successfully added.

TheShermanTanker · 2023-08-17T04:45:26Z

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

mlbridge · 2023-08-17T04:45:55Z

Webrevs

TheShermanTanker · 2023-08-17T04:46:05Z

/reviewers 3 reviewer

openjdk · 2023-08-17T04:47:26Z

@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).

tstuefe · 2023-08-17T05:59:19Z

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.

mlbridge · 2023-08-17T20:26:11Z

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

mlbridge · 2023-08-17T20:26:13Z

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

TheShermanTanker · 2023-08-18T06:39:54Z

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?

dholmes-ora · 2023-08-18T07:04:28Z

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

kimbarrett · 2023-08-19T07:00:03Z

make/autoconf/flags-cflags.m4

@@ -574,11 +574,11 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],

  # CXXFLAGS C++ language level for all of JDK, including Hotspot.
  if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang || test "x$TOOLCHAIN_TYPE" = xxlc; then
-    LANGSTD_CXXFLAGS="-std=c++14"
+    LANGSTD_CXXFLAGS="-std=c++17"


No, this change cannot be made yet. As noted in the prior JEP discussion, we need to wait for the aix-ppc
port maintainers to upgrade the compiler they are using. I happened to check last with them about their
progress. While progress has been made, they are not yet ready to throw that switch. Hoping to finish that
work sometime this fall.

This is resolved. JDK23 doesn't support the old compiler any more. The only supported AIX compiler is C++17 capable.

kimbarrett · 2023-08-19T07:17:27Z

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.

kimbarrett · 2023-08-19T07:20:15Z

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.

theRealAph · 2023-08-19T07:45:50Z

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?

TheRealMDoerr

Successfully tested on AIX with xlc 17.1.1. Works for us (SAP). Please check with others who might still use an older compiler (IBM).

bridgekeeper · 2023-09-18T15:18:38Z

@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!

bridgekeeper · 2024-04-17T11:41:18Z

@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!

MBaesken · 2024-04-17T13:13:00Z

Seems we use already a little bit of C++17 coding in the Linux codebase .
Just came across this little error when trying to build with clang on Linux

jdk/src/hotspot/os/linux/os_linux.cpp:2975:65: error: 'static_assert' with no message is a C++17 extension [-Werror,-Wc++17-extensions]
static_assert(MADV_POPULATE_WRITE == MADV_POPULATE_WRITE_value);

So switching to C++17 would make our codebase compile :-) (at least in this case) !
(to be more serious, I guess I better file a JBS bug and post a PR/fix for this static_assert)

magicus · 2024-04-24T09:18:05Z

@TheShermanTanker I suggest you close this PR. If we are going to switch to C++17, it should start by a discussion in the mailing list, not with a PR (the change itself is trivial).

bridgekeeper · 2024-06-19T11:57:33Z

@TheShermanTanker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!

bridgekeeper · 2024-08-14T17:56:43Z

@TheShermanTanker This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

TheShermanTanker · 2024-10-18T17:51:49Z

/open

openjdk · 2024-10-18T17:52:20Z

@TheShermanTanker This pull request is now open

TheRealMDoerr · 2024-10-18T19:09:50Z

There are build warnings on Windows (see GHA).

kimbarrett · 2024-10-18T21:59:41Z

There are build warnings on Windows (see GHA).

I recently noticed that problem too, and am working on what to do about it. I guess I should file a bug.

I think it wasn't previously noticed because there was a different bug that adlc wasn't being compiled with the
desired -std=c++14, which has since been fixed.

kimbarrett · 2024-10-18T22:15:55Z

There are build warnings on Windows (see GHA).

I recently noticed that problem too, and am working on what to do about it. I guess I should file a bug.
-std=c++14, which has since been fixed.

https://bugs.openjdk.org/browse/JDK-8342639
Global operator new in adlc has wrong exception spec

bridgekeeper · 2025-01-15T11:22:38Z

@TheShermanTanker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!

bridgekeeper · 2025-03-12T16:04:11Z

@TheShermanTanker This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

TheShermanTanker · 2025-06-26T07:05:02Z

Reopening since #25992 has been created for discussion on C++17 features. This Pull Request will be useful in helping to see what will need to change to actually enable C++17

TheShermanTanker · 2025-06-26T07:05:07Z

/open

openjdk · 2025-06-26T07:05:36Z

@TheShermanTanker This pull request is now open

8310260

d0405da

TheShermanTanker marked this pull request as draft July 24, 2023 01:41

openjdk bot changed the title ~~8310260~~ 8310260: Compile the JDK as C++17 Jul 24, 2023

openjdk bot added the build [email protected] label Jul 24, 2023

allocation.cpp

cdda85b

openjdk bot added the hotspot [email protected] label Jul 24, 2023

vm_version_linux_riscv.cpp

09cecd7

TheShermanTanker changed the title ~~8310260: Compile the JDK as C++17~~ 8314488 Aug 17, 2023

TheShermanTanker marked this pull request as ready for review August 17, 2023 04:39

openjdk bot changed the title ~~8314488~~ 8314488: Compile the JDK as C++17 Aug 17, 2023

openjdk bot added the rfr Pull request is ready for review label Aug 17, 2023

kimbarrett suggested changes Aug 19, 2023

View reviewed changes

TheRealMDoerr approved these changes Aug 21, 2023

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Mar 18, 2024

Merge branch 'master' into patch-7

9286a96

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Mar 20, 2024

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 10, 2024

Merge branch 'master' into patch-7

228c53a

TheShermanTanker marked this pull request as draft May 6, 2024 04:50

openjdk bot removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels May 6, 2024

bridgekeeper bot closed this Aug 14, 2024

openjdk bot reopened this Oct 18, 2024

Merge branch 'master' into patch-7

7e4ba76

bridgekeeper bot closed this Mar 12, 2025

openjdk bot reopened this Jun 26, 2025

8314488: Compile the JDK as C++17 #14988

Are you sure you want to change the base?

8314488: Compile the JDK as C++17 #14988

Conversation

TheShermanTanker commented Jul 24, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Integration blocker

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 24, 2023

Uh oh!

openjdk bot commented Jul 24, 2023

Uh oh!

TheShermanTanker commented Jul 24, 2023

Uh oh!

openjdk bot commented Jul 24, 2023

Uh oh!

TheShermanTanker commented Aug 17, 2023

Uh oh!

mlbridge bot commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheShermanTanker commented Aug 17, 2023

Uh oh!

openjdk bot commented Aug 17, 2023

Uh oh!

tstuefe commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Aug 17, 2023

Uh oh!

mlbridge bot commented Aug 17, 2023

Uh oh!

TheShermanTanker commented Aug 18, 2023

Uh oh!

dholmes-ora commented Aug 18, 2023

Uh oh!

kimbarrett Aug 19, 2023

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr Oct 18, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimbarrett commented Aug 19, 2023

Uh oh!

kimbarrett commented Aug 19, 2023

Uh oh!

theRealAph commented Aug 19, 2023

Uh oh!

TheRealMDoerr left a comment • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Sep 18, 2023

Uh oh!

bridgekeeper bot commented Apr 17, 2024

Uh oh!

MBaesken commented Apr 17, 2024

Uh oh!

magicus commented Apr 24, 2024

Uh oh!

bridgekeeper bot commented Jun 19, 2024

Uh oh!

bridgekeeper bot commented Aug 14, 2024

Uh oh!

TheShermanTanker commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

TheRealMDoerr commented Oct 18, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimbarrett commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheShermanTanker commented Jul 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 17, 2023 •

edited

Loading

tstuefe commented Aug 17, 2023 •

edited

Loading

TheRealMDoerr Oct 18, 2024 •

edited by bridgekeeper bot

Loading

TheRealMDoerr left a comment •

edited by bridgekeeper bot

Loading

TheRealMDoerr commented Oct 18, 2024 •

edited by bridgekeeper bot

Loading

kimbarrett commented Oct 18, 2024 •

edited

Loading