-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 #11115
Conversation
👋 Welcome back xuelei! A progress list of the required criteria for merging this PR into |
@XueleiFan The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @XueleiFan,
could you use jio_snprintf
instead (see include/jvm_io.h)? That is what we usually do for snprintf. jio_snprintf hides platform particularities wrt snprintf.
Cheers, Thomas
Good to know that. Thank you! While I was doing the replacement from
|
Hmm, possibly. We may look again at the exact reason why we use jio_snprintf. Maybe it is less important nowadays, with reduced platform number (no solaris) and Windows being more standard conform than it had been in the past. Lets hear what others think. |
Please don't add uses of Regarding I think the only reason we haven't marked As a general note, as a reviewer my preference is against non-trivial and |
Updated to use os::snprintf, except the files under adlc where the os::snptintf definition is not included. The use of snprintf could be cleaned up with existing code in the future.
It makes sense to me. I'd better focus on the building issue in this PR. Thank you for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hotspot changes seem okay using os::snprint. The adlc changes to use raw snprintf also seem okay for now - I'm not sure whether the platform differences for snprintf affect adlc.
The desktop change is wrong - you can't use os::snprintf there.
Thanks.
src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp
Outdated
Show resolved
Hide resolved
I did not know this was our policy now. Sorry for giving the wrong advice. Maybe we should add this to the hotspot style guide since I'm probably not the only one not knowing this.
I agree with you. Makes backporting a bit easier too. |
Kim said:
There's a lot of wisdom in what you say. It's far too easy to mess things up when doing cleanups for compiler warnings. Also, long patches never get enough reviewing. |
Please review the last update, and hopefully we are close to an agreement. Thanks! |
Any update on this? xcode 14 build has been broken for a while. |
Could I have the last update reviewed? |
I will have a look, I am affected too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through all the code and found only some minor issues. I would appreciate more eyes on this, though.
I still think this would have been less work (for author and reviewers) had we converted the code to stringStream right away in the hotspot. stringStream takes care of a lot of this boilerplate stuff.
Mailing list message from Michael Hall on client-libs-dev:
It builds for me? * Toolchain: clang (clang/LLVM from Xcode 14.1) With the exception of these errors in cmstypes.c /Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:3441:132: error: parameter 'SizeOfTag' set but not used [-Werror,-Wunused-but-set-parameter] I had seen this sometime back. The same workaround of adding? cmsUNUSED_PARAMETER(SizeOfTag); // mjh To the two methods, which I had noticed included elsewhere in the code, still appears to work. I first noticed the sprintf issue in later releases of Xcode 13. It isn?t just Xcode 14. -------------- next part -------------- |
The SizeOfTag issue was tracked with https://bugs.openjdk.org/browse/JDK-8283221.
In the Apple Developer Documentation, there is a note for sprintf, "macOS 10.12–10.13.1 Deprecated". It looks like that deprecation was backported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good from my side. But someone else should look as well. It's fiddly stuff.
I will try building openjdk20 with this patch tonight on mbp 2015, macos 13, xcode 14... |
Mailing list message from Michael Hall on client-libs-dev:
Sorry, I should of mentioned that I had come across this bug report the first time I had this issue. I don?t think it was closed at the time. The --disable-warnings-as-errors seemed the only suggested workaround at the time. I thought mine was better. I?m not very familiar with these things but I assume that the fix for that bug hasn?t been merged into your pull request yet, but will be at some point.
I installed an old Xcode (13.1) version. Which wasn?t a very easy process and probably only was possible since I have an Apple developer account to get the download. The next time I had gone into Xcode it forced an update to 14. I have continued to use the 13.1 to allow jdk builds until trying yours with 14.1. It worked as indicated. Thanks for the reply. -------------- next part -------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of minor nits that you can address or not.
LGTM, build on macos 13 + xcode 14.1 (x64): OK I run successfully few applications with built jdk image...
|
/integrate |
Going to push as commit 478ef38.
Your commit was automatically rebased without conflicts. |
@XueleiFan Pushed as commit 478ef38. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Mailing list message from Michael Hall on client-libs-dev: Fwiw, Where I seem to differ? macOS 12.6 * Version string: 20-internal-adhoc.mjh.jdk (20-internal) * Boot JDK: openjdk version "19" 2022-09-20 OpenJDK Runtime Environment (build 19+36-2238) OpenJDK 64-Bit Server VM (build 19+36-2238, mixed mode, sharing) (at /Library/Java/JavaVirtualMachines/jdk-19.jdk/Contents/Home) But it did build without the cmstypes errors. === Output from failing command(s) repeated here === |
I just stumbled across this issue as well, make images did succeed, only the test is now failing with the same error
|
JDK-8299378 was filled to track the issue. Thanks! |
Mailing list message from Michael Hall on client-libs-dev: Thank you for providing the tracking issue. |
Hi,
May I have this update reviewed?
The sprintf is deprecated in Xcode 14 because of security concerns, and the use of it causing building failure. The build could pass if warnings are disabled for codes that use sprintf method. For the long run, the sprintf could be replaced with snprintf. This patch is trying to check if snprintf could be used.
Thanks,
Xuelei
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11115/head:pull/11115
$ git checkout pull/11115
Update a local copy of the PR:
$ git checkout pull/11115
$ git pull https://git.openjdk.org/jdk pull/11115/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11115
View PR using the GUI difftool:
$ git pr show -t 11115
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11115.diff