JDK-8257989: Error in gtest os_page_size_for_region_unaligned after 8257588

[mgkwill]

There appears to be a bug at ln105 of test/hotspot/gtest/runtime/test_os.cpp

https://github.com/openjdk/jdk/pull/1522/files/163b308b97c07d567c48f100475481cdc5e75740#diff-6d3fc66964a0fccf7f85c284fffec5dffa62be8497423a7684cee83d55338884R104

// Given a slightly smaller size than a page size, // return the next smaller page size. if (os::_page_sizes[1] > os::_page_sizes[0]) { size_t expected = os::_page_sizes[0]; size_t actual = os::page_size_for_region_unaligned(os::_page_sizes[1] - 17, 1); ASSERT_EQ(actual, expected); for (size_t s = os::page_sizes().largest(); s != 0; s = os::page_sizes().next_smaller(s)) { const size_t expected = os::page_sizes().next_smaller(s); if (expected != 0) { size_t actual = os::page_size_for_region_unaligned(expected - 17, 1); ASSERT_EQ(actual, expected); } }

On ln104 we are trying to compare the next smaller page size of 's', with a page size returned for slightly smaller size of the size 's -17'.

Instead we compare next smaller page size of 's' to 'expected - 17'. I believe this should be 's - 17':
os::page_size_for_region_unaligned(expected - 17, 1);
vs.
os::page_size_for_region_unaligned(s - 17, 1);

Running gtest tests with 2 largepage sizes (3 total sizes) fails, however default of 2 page_sizes passes (4k and 2m or 1g):
(./hotspot/variant-server/libjvm/gtest/gtestLauncher -jdk:./images/jdk -Xms2G -Xmx2G -XX:+UseLargePages -XX:LargePageSizeInBytes=2M)

[----------] 17 tests from os [ RUN ] os.page_size_for_region_vm [ OK ] os.page_size_for_region_vm (0 ms) [ RUN ] os.page_size_for_region_aligned_vm [ OK ] os.page_size_for_region_aligned_vm (0 ms) [ RUN ] os.page_size_for_region_alignment_vm [ OK ] os.page_size_for_region_alignment_vm (0 ms) [ RUN ] os.page_size_for_region_unaligned_vm test/hotspot/gtest/runtime/test_os.cpp:106: Failure Expected equality of these values: actual Which is: 4096 expected Which is: 2097152 [ FAILED ] os.page_size_for_region_unaligned_vm (0 ms)

This only happen when #1153 is present in code, because otherwise you will only have two page_sizes, but still this should return the correct results.

Signed-off-by: Marcus G K Williams marcus.williams@intel.com

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8257989: Error in gtest os_page_size_for_region_unaligned after 8257588

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1719/head:pull/1719
$ git checkout pull/1719

[bridgekeeper]

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

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

[mgkwill]

/reviewer @tstuefe

[openjdk]

@mgkwill Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

• /reviewer credit @openjdk-bot
• /reviewer credit duke
• /reviewer credit @user1 @user2

[mgkwill]

/reviewer @tstuefe

[openjdk]

@mgkwill Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

• /reviewer credit @openjdk-bot
• /reviewer credit duke
• /reviewer credit @user1 @user2

[mlbridge]

Webrevs

• 00: Full (0af7df1)

[tstuefe]

Hi,

Fix makes sense and is trivial.

I opened https://bugs.openjdk.java.net/browse/JDK-8257989 for this.

("Trivial" means only needs one reviewer and can be integrated right away without waiting the obligatory 24hrs).

But please change the title of the PR to "JDK-8257989: Error in gtest os_page_size_for_region_unaligned after 8257588" before pushing, otherwise skara gets confused.

[tstuefe]

LGTM. Thanks for finding this!

[openjdk]

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

8257989: Error in gtest os_page_size_for_region_unaligned after 8257588

Reviewed-by: stuefe

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

• 5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
• 6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
• bd22aa5: 8229862: NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:570)
• cf62b0a: 8257518: LogCompilation: java.lang.InternalError with JFR turned on
• 6c69eca: 8257973: UTIL_LOOKUP_PROGS should only find executable files

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@tstuefe) 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).

[mgkwill]

/integrate

[openjdk]

@mgkwill
Your change (at version 0af7df1) is now ready to be sponsored by a Committer.

[tstuefe]

/sponsor

[openjdk]

@tstuefe @mgkwill Since your change was applied there have been 5 commits pushed to the master branch:

• 5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
• 6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
• bd22aa5: 8229862: NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:570)
• cf62b0a: 8257518: LogCompilation: java.lang.InternalError with JFR turned on
• 6c69eca: 8257973: UTIL_LOOKUP_PROGS should only find executable files

Your commit was automatically rebased without conflicts.

Pushed as commit b977a7b.

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