8324307: [11u] hotspot fails to build with GCC 12 and newer (non-static data member initializers)

[jmtd]

GCC 12 and newer warn about non-static data member initialisers when operating under -std=gnu++98 (jdk11u-dev's C++ standards version). These warnings are promoted to errors by default.

This is an issue for jdk11u-dev which uses the C++ standard version '-std=gnu++98'. It is not an issue for later JDK versions which have moved to newer standards (JEP 347 moved JDK16 to -std=c++14): therefore an 11u-specific fix is required.

The approach used in this PR is to remove the initialisers, in common with what we did when backporting the same code to jdk8u-dev.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8324307 needs maintainer approval

Issue

• JDK-8324307: [11u] hotspot fails to build with GCC 12 and newer (non-static data member initializers) (Bug - P4 - Approved)

Reviewers

• Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2470/head:pull/2470
$ git checkout pull/2470

Update a local copy of the PR:
$ git checkout pull/2470
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2470/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2470

View PR using the GUI difftool:
$ git pr show -t 2470

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2470.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jdowland! 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.

[mlbridge]

Webrevs

• 00: Full (550e7100)

[jerboaa]

OK.

[openjdk]

⚠️ @jmtd This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[jmtd]

/approval request to fix building 11u-dev with GCC 12+. 11u-specific fix needed due to later JDKs moving to a newer C++ standards version. TIA!

[openjdk]

@jmtd
8324307: The approval request has been created successfully.

[GoeLin]

@jmtd, if these initializers are not needed at all, shouldn't they be removed in the higher releases as well?

[GoeLin]

Removing the tag in the meantime

[jmtd]

Hi @GoeLin ,

@jmtd, if these initializers are not needed at all, shouldn't they be removed in the higher releases as well?

This is a really good question and I took some time to ponder my reply.

They are not needed from a computational POV because the relevant fields are all initialised in the object constructors in all cases. However, to know this I had to read through the class definition to be sure. I didn't write the original code but if I had I would have likely also preferred an explicit null initialisation in the member declaration, which makes clear the intent that we do not have uninitialised fields and do not risk reading from uninitialised fields. There's a chance the constructor could be altered later and an iniitialisation accidentally removed, or an alternative constructor added lacking the initialisation, etc.

I doubt that anyone would welcome a patch to remove the redundant initialisers in main, but I could propose one if you disagree. WDYT?

[gnu-andrew]

Hi @GoeLin ,

@jmtd, if these initializers are not needed at all, shouldn't they be removed in the higher releases as well?

This is a really good question and I took some time to ponder my reply.

They are not needed from a computational POV because the relevant fields are all initialised in the object constructors in all cases. However, to know this I had to read through the class definition to be sure. I didn't write the original code but if I had I would have likely also preferred an explicit null initialisation in the member declaration, which makes clear the intent that we do not have uninitialised fields and do not risk reading from uninitialised fields. There's a chance the constructor could be altered later and an iniitialisation accidentally removed, or an alternative constructor added lacking the initialisation, etc.

I doubt that anyone would welcome a patch to remove the redundant initialisers in main, but I could propose one if you disagree. WDYT?

I would say this is the sort of change that belongs only in 11u and older, for compatibility reasons (old C/C++ standards, older compilers). The current code in 17u+ is fine with -std=c++14.

Incidentally, if we are fixing 11u to compile with GCC 12, should we not also enable a GHA build with GCC 12 in a separate PR so that it does not regress again?

[jmtd]

I'll re-label approval-request, a I doubt Goetz saw my comment (or yours @gnu-andrew )

/approval request rationale for change added in #2470 (comment) , please take another look!

@gnu-andrew :

Incidentally, if we are fixing 11u to compile with GCC 12, should we not also enable a GHA build with GCC 12 in a separate PR so that it does not regress again?

Makes sense to me. I guess that would start with a new 11u-specific JDK bug; I'll file one.

[openjdk]

@jmtd
8324307: The approval request has been updated successfully.

[gnu-andrew]

I'll re-label approval-request, a I doubt Goetz saw my comment (or yours @gnu-andrew )

I think #2451 needs to be re-labelled as well. I'm not a fan of removing these requests outside the process used by the approval/approve commands as it isn't clear what's going on from the PR.

[openjdk]

@jmtd This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8324307: [11u] hotspot fails to build with GCC 12 and newer (non-static data member initializers)

Reviewed-by: sgehwolf

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 39 new commits pushed to the master branch:

• 621048a: 8247818: GCC 10 warning stringop-overflow with symbol code
• a60fb5a: 8325150: (tz) Update Timezone Data to 2024a
• 1f85c15: 8318608: Enable parallelism in vmTestbase/nsk/stress/threads tests
• 5aa8538: 8318607: Enable parallelism in vmTestbase/nsk/stress/jni tests
• 54d1857: 8325096: Test java/security/cert/CertPathBuilder/akiExt/AKISerialNumber.java is failing
• 4c25b22: 8287113: JFR: Periodic task thread uses period for method sampling events
• 9be4891: 8323243: JNI invocation of an abstract instance method corrupts the stack
• 77c0da4: 8306683: Open source several clipboard and color AWT tests
• 38c11fd: 8315415: OutputAnalyzer.shouldMatchByLine() fails in some cases
• 0dc17ca: 8315731: Open source several Swing Text related tests
• ... and 29 more: https://git.openjdk.org/jdk11u-dev/compare/03cd88522ed4af7d0c008abd6dedf70ddd37e253...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[jmtd]

/integrate

[openjdk]

Going to push as commit 1f516f6.
Since your change was applied there have been 41 commits pushed to the master branch:

• bcdad92: 8249087: Always initialize _body[0..1] in Symbol constructor
• daf393a: 8304725: AsyncGetCallTrace can cause SIGBUS on M1
• 621048a: 8247818: GCC 10 warning stringop-overflow with symbol code
• a60fb5a: 8325150: (tz) Update Timezone Data to 2024a
• 1f85c15: 8318608: Enable parallelism in vmTestbase/nsk/stress/threads tests
• 5aa8538: 8318607: Enable parallelism in vmTestbase/nsk/stress/jni tests
• 54d1857: 8325096: Test java/security/cert/CertPathBuilder/akiExt/AKISerialNumber.java is failing
• 4c25b22: 8287113: JFR: Periodic task thread uses period for method sampling events
• 9be4891: 8323243: JNI invocation of an abstract instance method corrupts the stack
• 77c0da4: 8306683: Open source several clipboard and color AWT tests
• ... and 31 more: https://git.openjdk.org/jdk11u-dev/compare/03cd88522ed4af7d0c008abd6dedf70ddd37e253...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jmtd Pushed as commit 1f516f6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.