8348536: Remove remain SIZE_FORMAT usage after JDK-8347990

[sendaoYan]

Hi all,
This is complement implement PR of JDK-8347990, which fix make fails with extra flags '-DLOG_PLEASE'.

Change has been verified locally, test-fix only, risk is low.

---

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-8348536: Remove remain SIZE_FORMAT usage after JDK-8347990 (Bug - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23290

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back syan! 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]

@sendaoYan 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:

8348536: Remove remain SIZE_FORMAT usage after JDK-8347990

Reviewed-by: dholmes, kbarrett

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

• 990d40e: 8350476: Fix typo introduced in JDK-8350147
• 39cb493: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
• ddb2569: 8280682: Refactor AOT code source validation checks
• 65f79c1: 8347335: ZGC: Use limitless mark stack memory
• e410af0: 8342393: Promote commutative vector IR node sharing
• f755fad: 8349653: Clarify the docs for MemorySegment::reinterpret
• a5c9a4d: 8349032: C2: Parse Predicate refactoring in Loop Unswitching broke fix for JDK-8290850
• 302bed0: 8350499: Minimal build fails with slowdebug builds
• 0795d11: 8350464: The flags to set the native priority for the VMThread and Java threads need a broader range
• 05b4812: 8350041: Skip test/jdk/java/lang/String/nativeEncoding/StringPlatformChars.java on static JDK
• ... and 339 more: https://git.openjdk.org/jdk/compare/416d46947ca926a31fbf9e97d0e7c320063ecfb2...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.

[openjdk]

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

• hotspot-runtime

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.

[mlbridge]

Webrevs

• 03: Full - Incremental (806b5751)
• 02: Full - Incremental (2fc808b2)
• 01: Full - Incremental (4d7440ce)
• 00: Full (af297c41)

[bridgekeeper]

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

[sendaoYan]

Hi, can anyone take look this PR.

[dholmes-ora]

Lots of gtests attempt to use SIZE_FORMAT. Isn't this just a missing include of globalDefinitions ??

[sendaoYan]

Lots of gtests attempt to use SIZE_FORMAT. Isn't this just a missing include of globalDefinitions ??

Most of the use of SIZE_FORMAT has been removed by #23180

[dholmes-ora]

Okay. I am still seeing a number of other uses in the gtests so I guess @coleenp missed a few others.

[kimbarrett]

Lots of gtests attempt to use SIZE_FORMAT. Isn't this just a missing include of globalDefinitions ??

There are no longer definitions of SIZE_FORMAT and SSIZE_FORMAT in globalDefinitions.hpp.

Presumably the uses being looked at here (which appear to be all of the remaining residue) were overlooked
because they were under conditional compilation that doesn't didn't get tested.

Running egrep -r "\bS?SIZE_FORMAT\b" . in src/hotspot gets nothing. Running it in test/hotspot/gtest finds
the ones being changed by this PR.

[kimbarrett]

make fails with extra flags '-DLOG_PLEASE'

The way that macro is being used, I don't think it's intended to be globally enabled like that. It's definition is typically
commented or uncommented to provide some additional logging for some tests of interest. I'm wondering if the places
where it is currently being defined are debugging leftovers that shouldn't have been committed.

For example, I think the LOG_PLEASE and LOG_HERE in runtime/test_os_reserve_between.cpp should either comment
out the LOG_PLEASE definition or just use ordinary logging. Maybe @tstuefe has an opinion?

Maybe the changes related to LOG_PLEASE definitions ought to be removed and examined as a separate issue?
It's still good that the lingering SIZE_FORMATs were uncovered and are being fixed.

[sendaoYan]

Maybe the changes related to LOG_PLEASE definitions ought to be removed and examined as a separate issue? It's still good that the lingering SIZE_FORMATs were uncovered and are being fixed.

The LOG_PLEASE definition has been removed by this PR, and I create a new issue JDK-8350584 to record this issue.

[coleenp]

I didn't see these with the final grep and now I see they're conditionally compiled out. There are a couple others here. Can you get these too?

gtest/nmt/test_nmt_totals.cpp:  LOG("Deviation: n=" SSIZE_FORMAT ", s=" SSIZE_FORMAT ", ovrh=" SSIZE_FORMAT,   \
gtest/runtime/test_virtualMemoryTracker.cpp:  LOG("In reserved region " PTR_FORMAT ", size " SIZE_FORMAT_HEX ":", p2i(rmr->base()), rmr->size());
gtest/runtime/test_virtualMemoryTracker.cpp:    LOG("   committed region: " PTR_FORMAT ", size " SIZE_FORMAT_HEX, p2i(region->base()), region->size());

[sendaoYan]

I didn't see these with the final grep and now I see they're conditionally compiled out. There are a couple others here. Can you get these too?

Do you mean I should replace SSIZE_FORMAT as %zd in this PR?
What's string should I use to replace to PTR_FORMAT and PTR_FORMAT.

[coleenp]

Yes, replace SSIZE_FORMAT with %zd. I see that the SIZE_FORMAT_HEX was the one I kept, so that should not be replaced. Sorry for the confusion. PTR_FORMAT should not be replaced.

Edit. I'm the one suffering from confusion. I don't see any SIZE_FORMAT_HEX, just SIZE_FORMAT_X_0. Can you fix that one too?

[sendaoYan]

Yes, replace SSIZE_FORMAT with %zd.

SSIZE_FORMAT has been replace as %zd.

[kimbarrett]

Looks good.

[dholmes-ora]

Okay so this is no longer about LOG_PLEASE but just the leftover SIZE_FORMAT.

The way that macro is being used, I don't think it's intended to be globally enabled like that.

I don't see why not, but I will take that up in the other PR.

[dholmes-ora]

Seems fine. Thanks

[sendaoYan]

Thanks @kimbarrett @coleenp @dholmes-ora for the suggestions and reviews.

/integrate

[openjdk]

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

• a6cc37f: 8349888: AOTMode=create crashes with EpsilonGC
• 990d40e: 8350476: Fix typo introduced in JDK-8350147
• 39cb493: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
• ddb2569: 8280682: Refactor AOT code source validation checks
• 65f79c1: 8347335: ZGC: Use limitless mark stack memory
• e410af0: 8342393: Promote commutative vector IR node sharing
• f755fad: 8349653: Clarify the docs for MemorySegment::reinterpret
• a5c9a4d: 8349032: C2: Parse Predicate refactoring in Loop Unswitching broke fix for JDK-8290850
• 302bed0: 8350499: Minimal build fails with slowdebug builds
• 0795d11: 8350464: The flags to set the native priority for the VMThread and Java threads need a broader range
• ... and 340 more: https://git.openjdk.org/jdk/compare/416d46947ca926a31fbf9e97d0e7c320063ecfb2...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sendaoYan Pushed as commit e1081cf.

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

[coleenp]

I requested since you were doing this, that you should fix SIZE_FORMAT_HEX in test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp too.

[sendaoYan]

I requested since you were doing this, that you should fix SIZE_FORMAT_HEX in test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp too.

Sorry, I did not got your request exately. Do you mean the SIZE_FORMAT_HEX should be deal with? What's flags should be replace to SIZE_FORMAT_HEX

[coleenp]

From the test_globalDefinitions.cpp, this seems like a good format for that. If you want to make a trivial change.
check_format("0x%zx", (intx)0x123, "0x123");

[sendaoYan]

From the test_globalDefinitions.cpp, this seems like a good format for that. If you want to make a trivial change. check_format("0x%zx", (intx)0x123, "0x123");

I have create a new PR #23778 to fix remain SIZE_FORMAT_HEX macro undefined issue since this PR has been integrated.