8198918: jio_snprintf and friends are not checked by -Wformat #15918
Conversation
|
👋 Welcome back azafari! A progress list of the required criteria for merging this PR into |
|
@afshin-zafari 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. |
| if (BootstrapJVMCI && (TieredStopAtLevel < CompLevel_full_optimization)) { | ||
| jio_fprintf(defaultStream::error_stream(), | ||
| "-XX:+BootstrapJVMCI is not compatible with -XX:TieredStopAtLevel=%d\n", TieredStopAtLevel); | ||
| "-XX:+BootstrapJVMCI is not compatible with -XX:TieredStopAtLevel=" INTX_FORMAT "\n", TieredStopAtLevel); |
There was a problem hiding this comment.
The real problem here is that TieredStopAtLevel is inappropriately declared to be an intx.
There was a problem hiding this comment.
This change required changes to 18 Java files and then many test failures in mach5 tiers tests. So, I removed the changes to avoid diverging from this PR.
src/hotspot/share/runtime/os.cpp
Outdated
| const int printed = jio_snprintf(buffer, buffer_length, | ||
| "%04d-%02d-%02dT%02d:%02d:%02d.%03d%c%02d%02d", | ||
| const int printed = os::snprintf(buffer, buffer_length, | ||
| "%04d-%02d-%02dT%02d:%02d:%02d.%03d%c%02ld%02ld", |
There was a problem hiding this comment.
This is making the unwarranted assumption that time_t == long. Also, the "l" length modifier should
probably never be used in shared code, just as the long type shouldn't be used, because of the LP vs LLP
issue. Better would be to declare zone_hours and zone_min to be int, and explicitly convert the initial
value expressions to int.
There was a problem hiding this comment.
Both are wrong. You can use %02d but must explicitly cast to int (or to long if you use %02ld but that makes no sense for a two-digit number) because time_t is often wider than long… which -Wformat would have shown.
There was a problem hiding this comment.
Ah, nevermind, I see now that the PR was changed to change the variable type instead… which is fair in this situation.
| if (ReservedCodeCacheSize < InitialCodeCacheSize) { | ||
| jio_fprintf(defaultStream::error_stream(), | ||
| "Invalid ReservedCodeCacheSize: %dK. Must be at least InitialCodeCacheSize=%dK.\n", | ||
| "Invalid ReservedCodeCacheSize: " UINTX_FORMAT "K. Must be at least InitialCodeCacheSize=" UINTX_FORMAT "K.\n", |
There was a problem hiding this comment.
It seems like all these size values ought to be size_t rather than uintx, in which case could use "%zu".
I don't know what the fannout for changing the types might be.
There was a problem hiding this comment.
Fixed. There were also some related changes in JTREG Java files.
|
@kimbarrett, |
|
I close this PR since the solution is not applicable to the |
|
Why did this not add the ATTRIBUTE_PRINTF to |
3 participants
ATTRIBUTE_PRINTFusage in cpp files is useless. They are removed.jio_xxprintffunctions usechar *arguments for format string, rather than a literal like"%s...". These cases are not compiled whenATTRIBUTE_PRINTFis used for them. So, first I used the attribute and got the corresponding compile errors. Then I fixed the issues and removed the attribute when all issues were fixed.Test
The changes are tested on all platforms tiers 1-4.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15918/head:pull/15918$ git checkout pull/15918Update a local copy of the PR:
$ git checkout pull/15918$ git pull https://git.openjdk.org/jdk.git pull/15918/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15918View PR using the GUI difftool:
$ git pr show -t 15918Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15918.diff
Webrev
Link to Webrev Comment