JDK-8324243: Compilation failures in java.desktop module with gcc 14 #17506

thesamesam · 2024-01-20T10:15:02Z

This fixes building with GCC 14:

~~Cherry-pick a fix from Harfbuzz upstream~~
Apply other -Wcalloc-transposed-args fixes to the JDK sources

-Wcalloc-transposed-args errors out with GCC 14 as the OpenJDK build uses
-Werror.

The calloc prototype is:

void *calloc(size_t nmemb, size_t size);

So, just swap the number of members and size arguments to match the prototype, as
we're initialising 1 struct of size sizeof(struct ...). GCC then sees we're not
doing anything wrong.

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

Issue

JDK-8324243: Compilation failures in java.desktop module with gcc 14 (Bug - P4)

Reviewers

Julian Waters (@TheShermanTanker - Committer)
Magnus Ihse Bursie (@magicus - Reviewer)
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to ef698d82
Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17506/head:pull/17506
$ git checkout pull/17506

Update a local copy of the PR:
$ git checkout pull/17506
$ git pull https://git.openjdk.org/jdk.git pull/17506/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17506

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17506.diff

Webrev

Link to Webrev Comment

Backport harfbuzz/harfbuzz@032c931 to fix build with GCC 14.

bridgekeeper · 2024-01-20T10:16:02Z

👋 Welcome back thesamesam! 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 · 2024-01-20T10:17:37Z

@thesamesam The following label will be automatically applied to this pull request:

client

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 · 2024-01-21T04:57:26Z

@thesamesam Unfortunately we can't fix the harfbuzz sources, since they are third party code. Your changes to harfbuzz code wouldn't be accepted by the client team :(

thesamesam · 2024-01-21T05:05:03Z

@thesamesam Unfortunately we can't fix the harfbuzz sources, since they are third party code. Your changes to harfbuzz code wouldn't be accepted by the client team :(

Bleh. I don't suppose any exception can be made given it's a cherry-pick (i.e. my changes there are not original, they're upstream, just not in a release)?

If not, I can just drop that bit, and we can wait until a new Harfbuzz release is made and propagates into OpenJDK.

TheShermanTanker · 2024-01-21T05:11:15Z

I don't believe harfbuzz has been bumped into a specific commit before, so I'm not sure about that. Perhaps you could wait till Phil sees this, for a conclusive answer

thesamesam · 2024-01-21T05:15:53Z

ok - if I have to drop it, so be it, not the end of the world. I guess it's also tricky because of cherry-picking one change vs bumping to some git commit upstream as a whole. I'll wait a little bit then just drop if nobody chimes in. Thank you!

TheShermanTanker · 2024-01-21T05:23:57Z

You'd need a issue on the tracker to connect to this Pull Request for anyone to see this (most reviewers don't go through these manually, they come from the mailing lists). Are you able to create one yourself, or should I create one for you?

thesamesam · 2024-01-21T05:29:13Z

Would you be so kind to file one for me please? I don't have access yet.

TheShermanTanker · 2024-01-21T07:09:54Z

No problem!

Just change your Pull Request title to 8324243 (The literal number) and watch the magic happen :)

By the way, your name strikes me as familiar. Have I seen you before on the gcc mailing lists?

-Wcalloc-transposed-args errors out with GCC 14 as the OpenJDK build uses -Werror. The calloc prototype is: ``` void *calloc(size_t nmemb, size_t size); ``` So, just swap the number of members and size arguments to match the prototype, as we're initialising 1 struct of size `sizeof(struct ...)`. GCC then sees we're not doing anything wrong. Signed-off-by: Sam James <sam@gentoo.org>

openjdk · 2024-01-21T16:35:07Z

@thesamesam Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2024-01-21T16:38:36Z

Webrevs

thesamesam · 2024-01-21T16:42:40Z

No problem!

Many thanks!

Just change your Pull Request title to 8324243 (The literal number) and watch the magic happen :)

By the way, your name strikes me as familiar. Have I seen you before on the gcc mailing lists?

You know, now you say it, I think we have interacted before actually - either on the GCC ML or bugzilla! I tend to pop up in a lot of places just because we have a lot of users with a lot of different odd and interesting environments.

TheShermanTanker · 2024-01-21T16:51:14Z

No problem!

Many thanks!

Just change your Pull Request title to 8324243 (The literal number) and watch the magic happen :)
By the way, your name strikes me as familiar. Have I seen you before on the gcc mailing lists?

You know, now you say it, I think we have interacted before actually - either on the GCC ML or bugzilla! I tend to pop up in a lot of places just because we have a lot of users with a lot of different odd and interesting environments.

Ah! I think you might be the same Sam James in my thread about adding the invalid-noreturn warning to gcc (Which ultimately didn't get anywhere). Glad to meet you again! :)

This Pull Request might be related to client libs code, but given its nature I'd consider it a build-dev issue too, in which case, have a thumbs up from me, since that is my domain.

Thanks, and welcome to OpenJDK if you're new here!

thesamesam · 2024-01-21T16:55:45Z

Ah, yes! FWIW, I'm still disappointed about that as well (I don't have any sort of power, just tend to do a lot of triage). Do consider sending it again when stage 1 opens again after GCC 14 is out. Persistence is key with GCC, even if your change isn't an objectionable one. It took me a little while to get used to that..

src/java.desktop/share/native/libharfbuzz/hb-object.hh

This reverts commit acdd606c5d818baa783c6529ea58e66a061ec64a.

openjdk · 2024-02-02T06:29:55Z

@thesamesam Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

magicus · 2024-02-02T06:45:23Z

/label add build

openjdk · 2024-02-02T06:45:56Z

@magicus
The build label was successfully added.

magicus

Changes look good from a build perspective, but you need an ok from client as well for java.desktop changes.

kimbarrett

(I'm not a member of the relevant teams, but...)
The changes to calloc calls are plainly an improvement. The new gcc
warnings are correct in the sense of potentially confused code, though
in all of these cases I think it's "harmless".

Just a minor whitespace issue in the build changes.

make/modules/java.desktop/lib/Awt2dLibraries.gmk

prrace · 2024-02-21T22:24:01Z

@thesamesam you need to fix the PR title to match the JBS issue just as the bot is instructing you.
Then the bots should mark this "ready" to push.

thesamesam · 2024-02-22T01:41:01Z

Thanks, I hadn't gone back and read the original comment.

openjdk · 2024-02-22T01:42:06Z

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

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

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

8324243: Compilation failures in java.desktop module with gcc 14

Reviewed-by: jwaters, ihse, kbarrett, prr

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 385 new commits pushed to the master branch:

0bcece9: 8325342: Remove unneeded exceptions in compare.sh
64f7972: 8326158: Javadoc for java.time.DayOfWeek#minus(long)
f0f4d63: 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1
51e2dde: 8326235: RISC-V: Size CodeCache for short calls encoding
c022431: 8326412: debuginfo files should not have executable bit set
33834b7: 8326375: [REDO] Clean up NativeCompilation.gmk and its newly created parts
2352268: 8326334: JFR failed assert(used(klass)) failed: invariant
492e8bf: 8325587: Shenandoah: ShenandoahLock should allow blocking in VM
5f16f34: 8326370: Remove redundant and misplaced micros from StringBuffers
921507c: 8326319: G1: Remove unused G1ConcurrentMark::_init_times
... and 375 more: https://git.openjdk.org/jdk/compare/f4d08ccf80812d4f26a148fac6bf99b96672a63f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheShermanTanker, @magicus, @kimbarrett, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

thesamesam · 2024-02-22T01:42:25Z

/integrate

openjdk · 2024-02-22T01:43:06Z

@thesamesam
Your change (at version ef79e7a) is now ready to be sponsored by a Committer.

TheShermanTanker · 2024-02-22T06:26:22Z

Glad to have met you again in this Pull Request :)
/sponsor

openjdk · 2024-02-22T06:27:27Z

Going to push as commit 8e5f6dd.
Since your change was applied there have been 385 commits pushed to the master branch:

0bcece9: 8325342: Remove unneeded exceptions in compare.sh
64f7972: 8326158: Javadoc for java.time.DayOfWeek#minus(long)
f0f4d63: 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1
51e2dde: 8326235: RISC-V: Size CodeCache for short calls encoding
c022431: 8326412: debuginfo files should not have executable bit set
33834b7: 8326375: [REDO] Clean up NativeCompilation.gmk and its newly created parts
2352268: 8326334: JFR failed assert(used(klass)) failed: invariant
492e8bf: 8325587: Shenandoah: ShenandoahLock should allow blocking in VM
5f16f34: 8326370: Remove redundant and misplaced micros from StringBuffers
921507c: 8326319: G1: Remove unused G1ConcurrentMark::_init_times
... and 375 more: https://git.openjdk.org/jdk/compare/f4d08ccf80812d4f26a148fac6bf99b96672a63f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-22T06:27:33Z

@TheShermanTanker @thesamesam Pushed as commit 8e5f6dd.

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

thesamesam · 2024-02-22T06:41:37Z

Thank you!

harfbuzz: Cherry-pick upstream fix for GCC 14

0cb46f9

Backport harfbuzz/harfbuzz@032c931 to fix build with GCC 14.

openjdk bot added the client client-libs-dev@openjdk.org label Jan 20, 2024

thesamesam force-pushed the gcc14 branch from 6e36abe to cb5b9df Compare January 21, 2024 16:34

thesamesam changed the title ~~Fix GCC 14 build~~ 8324243: Fix GCC 14 build Jan 21, 2024

openjdk bot added the rfr Pull request is ready for review label Jan 21, 2024

TheShermanTanker approved these changes Jan 21, 2024

View reviewed changes

prrace reviewed Jan 23, 2024

View reviewed changes

src/java.desktop/share/native/libharfbuzz/hb-object.hh Show resolved Hide resolved

Conditionally pass -Wno-transposed-calloc-args for bundled harfbuzz

6734a00

openjdk bot removed the rfr Pull request is ready for review label Feb 2, 2024

Revert "harfbuzz: Cherry-pick upstream fix for GCC 14"

8ba3a7e

This reverts commit acdd606c5d818baa783c6529ea58e66a061ec64a.

thesamesam force-pushed the gcc14 branch from e97e001 to 8ba3a7e Compare February 2, 2024 06:29

fix whitespace

ef698d8

openjdk bot added the rfr Pull request is ready for review label Feb 2, 2024

openjdk bot added the build build-dev@openjdk.org label Feb 2, 2024

magicus approved these changes Feb 2, 2024

View reviewed changes

kimbarrett approved these changes Feb 2, 2024

View reviewed changes

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved

whitespace

ef79e7a

TheShermanTanker approved these changes Feb 4, 2024

View reviewed changes

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved

magicus approved these changes Feb 6, 2024

View reviewed changes

prrace approved these changes Feb 9, 2024

View reviewed changes

thesamesam changed the title ~~8324243: Fix GCC 14 build~~ JDK-8324243: Compilation failures in java.desktop module with gcc 14 Feb 22, 2024

openjdk bot added the ready Pull request is ready to be integrated label Feb 22, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 22, 2024

openjdk bot added the integrated Pull request has been integrated label Feb 22, 2024

openjdk bot closed this Feb 22, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 22, 2024

thesamesam deleted the gcc14 branch February 22, 2024 06:41

JDK-8324243: Compilation failures in java.desktop module with gcc 14 #17506

JDK-8324243: Compilation failures in java.desktop module with gcc 14 #17506

Uh oh!

Conversation

thesamesam commented Jan 20, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jan 20, 2024

Uh oh!

openjdk bot commented Jan 20, 2024

Uh oh!

TheShermanTanker commented Jan 21, 2024

Uh oh!

thesamesam commented Jan 21, 2024

Uh oh!

TheShermanTanker commented Jan 21, 2024

Uh oh!

thesamesam commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheShermanTanker commented Jan 21, 2024

Uh oh!

thesamesam commented Jan 21, 2024

Uh oh!

TheShermanTanker commented Jan 21, 2024

Uh oh!

openjdk bot commented Jan 21, 2024

Uh oh!

mlbridge bot commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

thesamesam commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheShermanTanker commented Jan 21, 2024

Uh oh!

thesamesam commented Jan 21, 2024

Uh oh!

Uh oh!

openjdk bot commented Feb 2, 2024

Uh oh!

magicus commented Feb 2, 2024

Uh oh!

openjdk bot commented Feb 2, 2024

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prrace commented Feb 21, 2024

Uh oh!

thesamesam commented Feb 22, 2024

Uh oh!

openjdk bot commented Feb 22, 2024

Uh oh!

thesamesam commented Feb 22, 2024

Uh oh!

openjdk bot commented Feb 22, 2024

Uh oh!

TheShermanTanker commented Feb 22, 2024

Uh oh!

openjdk bot commented Feb 22, 2024

Uh oh!

openjdk bot commented Feb 22, 2024

Uh oh!

thesamesam commented Feb 22, 2024

Uh oh!

Reviewers

Assignees

thesamesam commented Jan 20, 2024 •

edited by openjdk bot

Loading

thesamesam commented Jan 21, 2024 •

edited

Loading

mlbridge bot commented Jan 21, 2024 •

edited

Loading

thesamesam commented Jan 21, 2024 •

edited

Loading