Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8296812: sprintf is deprecated in Xcode 14 #2836

Closed
wants to merge 2 commits into from

Conversation

vieiro
Copy link
Contributor

@vieiro vieiro commented Jul 5, 2024

Backport of JDK-8296812 .

The new GHA build platform (macos-13 / XCode 14) fails because sprintf is deprecated. An alternative (os::snprintf_checked) was introduced in JDK-8296812.

This is sadly not a clean backport, as different issues have not been backported to 11 (JDK-8324824, JDK-8303575, JDK-8301050, JDK-8298472, JDK-8298472 to name a few).

The backport required changes to the following files:

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp
src/hotspot/os/bsd/os_bsd.cpp
src/hotspot/share/adlc/adlc.hpp
src/hotspot/share/adlc/adlparse.cpp
src/hotspot/share/adlc/formssel.cpp
src/hotspot/share/adlc/output_c.cpp
src/hotspot/share/c1/c1_Runtime1.cpp
src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp
src/hotspot/share/runtime/perfData.cpp
src/hotspot/share/utilities/utf8.cpp

And these files were not backported:

src/hotspot/share/cds/filemap.cpp
src/hotspot/share/interpreter/bootstrapInfo.cpp
src/hotspot/share/jfr/support/jfrSymbolTable.cpp

While we are at it, this change also replaces strcat in src/hotspot/cpu/aarch64/vm_version_aarch64.cpp with the newly introduced os::snprintf_checked to avoid possible buffer overflows.

Builds on the new macos-13 will continue to fail until different issues are backported. The new macos builds report a clash with the standard assert macro (see [1]) what suggests that JDK-8299254 "Support dealing with standard assert macro" should be backported next.

[1]

In file included from /Users/runner/work/jdk11u-dev/jdk11u-dev/test/hotspot/gtest/jfr/test_networkUtilization.cpp:223:
/Users/runner/work/jdk11u-dev/jdk11u-dev/src/hotspot/share/jfr/periodic/jfrNetworkUtilization.cpp:58:25: error: too many arguments provided to function-like macro invocation
  assert(iface != NULL, "invariant");

Progress

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

Warning

 ⚠️ Found leading lowercase letter in issue title for 8296812: sprintf is deprecated in Xcode 14

Issue

  • JDK-8296812: sprintf is deprecated in Xcode 14 (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2836

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 5, 2024

👋 Welcome back vieiro! 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
Copy link

openjdk bot commented Jul 5, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@vieiro vieiro changed the title Backports/jdk 8296812 Backport 605c9767291ddf1c409c3e805ffb3182899d06c2 Jul 5, 2024
@vieiro vieiro changed the title Backport 605c9767291ddf1c409c3e805ffb3182899d06c2 Backport 478ef389dc3767edfbe21d10a7f7f1522c648c2e Jul 5, 2024
@openjdk openjdk bot changed the title Backport 478ef389dc3767edfbe21d10a7f7f1522c648c2e 8296812: sprintf is deprecated in Xcode 14 Jul 5, 2024
@openjdk
Copy link

openjdk bot commented Jul 5, 2024

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Jul 5, 2024
@mlbridge
Copy link

mlbridge bot commented Jul 5, 2024

Webrevs

@vieiro vieiro changed the base branch from master to pr/2835 July 7, 2024 07:05
@openjdk-notifier openjdk-notifier bot changed the base branch from pr/2835 to master July 9, 2024 08:25
@openjdk-notifier
Copy link

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout backports/JDK-8296812
git fetch https://git.openjdk.org/jdk11u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@vieiro vieiro force-pushed the backports/JDK-8296812 branch from 76a5654 to e5d3f6e Compare July 9, 2024 08:40
@openjdk
Copy link

openjdk bot commented Jul 9, 2024

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

@vieiro
Copy link
Contributor Author

vieiro commented Jul 9, 2024

Apologies for the force push (no review comments yet).
I thought this could ease review, though, now that GHA actions are in place, also we verify latest commits don't affect this one.

@gdams
Copy link
Member

gdams commented Jul 16, 2024

Changes LGTM once src/hotspot/share/adlc/adlc.hpp is resolved

@TheRealMDoerr
Copy link
Contributor

#2835 should have been delayed until this fix is ready! Now, GHA is broken on MacOS!

@gdams
Copy link
Member

gdams commented Jul 17, 2024

#2835 should have been delayed until this fix is ready! Now, GHA is broken on MacOS!

it's been broken for weeks as there were no macos-11 agents available (it just left the CI in a pending state without completion)

@vieiro
Copy link
Contributor Author

vieiro commented Jul 17, 2024

@TheRealMDoerr Yep, the GHA macos-11 builders were deprecated on June the 28th, and all builds have been failing since then.

In any case after #2861 (and dependant PRs) the macos builds should be back to normal.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 14, 2024

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

@jerboaa
Copy link
Contributor

jerboaa commented Aug 22, 2024

@adinn Could you please help review this patch. In particular the changes in vm_version_aarch64.cpp? Thanks!

@jerboaa
Copy link
Contributor

jerboaa commented Aug 22, 2024

/reviewers 2

@openjdk
Copy link

openjdk bot commented Aug 22, 2024

@jerboaa
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

This seems OK to me.

@vieiro
Copy link
Contributor Author

vieiro commented Sep 5, 2024

Parent pull requests have been closed without review, so there's no point in continuing just with this PR.

@vieiro vieiro closed this Sep 5, 2024
@vieiro vieiro deleted the backports/JDK-8296812 branch September 5, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

4 participants