8367249: [REDO] MemBaseline accesses VMT without using lock #27170

jdksjolen · 2025-09-09T11:53:41Z

Hi,

This is a REDO, as the previous attempt caused build failures when merged with latest master.
The only difference is that the copy ctr for the RBTRee had been integrated with a different PR, but it never should have been. I deleted the copy ctr.

Thanks.

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-8367249: [REDO] MemBaseline accesses VMT without using lock (Bug - P4)

Reviewers

Afshin Zafari (@afshin-zafari - Reviewer)
Casper Norrbin (@caspernorrbin - Committer) Review applies to 9539fe96

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27170

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-09T11:54:59Z

👋 Welcome back jsjolen! 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 · 2025-09-09T11:57:10Z

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

8367249: [REDO] MemBaseline accesses VMT without using lock

Reviewed-by: azafari, cnorrbin

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 · 2025-09-09T11:57:58Z

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

hotspot

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.

afshin-zafari

LGTM. Thanks!

caspernorrbin

Still good!

mlbridge · 2025-09-09T12:04:25Z

Webrevs

jdksjolen · 2025-09-10T16:38:45Z

It's really weird that the linux-x64-static tests fail.

dholmes-ora · 2025-09-11T02:00:43Z

It's really weird that the linux-x64-static tests fail.

It just an infra issue:

Unable to download artifact(s): Artifact not found for name: bundles-jtreg-7.5.2+1

jdksjolen · 2025-09-11T07:41:11Z

It's really weird that the linux-x64-static tests fail.

It just an infra issue:

Unable to download artifact(s): Artifact not found for name: bundles-jtreg-7.5.2+1

Last time I ran it, it actually did fail on a Whitebox NMT test related to exactly this change. The logs were gone, so I re-ran the test, and this time infra failed.

jdksjolen · 2025-09-16T13:11:37Z

@afshin-zafari , I fixed another couple of bugs. Now I've gone through all of the VMT::Instance matches with grep and there seems to be no more issues.

linux-x64-static is still reporting an entirely empty Virtual memory map in NMT :-). Who knows why.

afshin-zafari · 2025-09-16T16:23:39Z

there seems to be no more issues.

The VMT::find_reserved_region(address p) (and the other methods that traversing the tree) also should be used within the NMT lock as well. Should we do them here in this PR or a separate one?

jdksjolen · 2025-09-17T07:42:49Z

there seems to be no more issues.

The VMT::find_reserved_region(address p) (and the other methods that traversing the tree) also should be used within the NMT lock as well. Should we do them here in this PR or a separate one?

I think that it's only the find_reserved_region left. It seems that this never took the lock, presumably because we are already killing the VM when this is called. I don't know whether or not this should take the lock, as I suspect the reasoning is a bit subtle. It's best to do this one in a separate PR, because of that.

jdksjolen · 2025-09-17T12:49:00Z

Hi @jianglizhou ,

There's a test failure that only appears in linux-x64-static tier1. I'm looking at running the tests under the static-jdk. So far, I've produced a static-jdk via make CONF=slow static-jdk-image. Do you happen to know how to execute the tier1 tests with such a JDK?

I've tried make JDK_UNDER_TEST=build/linux-x64-slowdebug/images/static-jdk/ test TEST=tier1 but it complains about JDK_UNDER_TEST not being a non-control variable.

jdksjolen · 2025-09-17T15:39:22Z

Well, that was a very embarrassing bug :-). I had code in an assert which needed to run regardless if in a debug build or not. Why didn't all release builds fail? No clue.

jianglizhou · 2025-09-18T00:04:59Z

Hi @jianglizhou ,

There's a test failure that only appears in linux-x64-static tier1. I'm looking at running the tests under the static-jdk. So far, I've produced a static-jdk via make CONF=slow static-jdk-image. Do you happen to know how to execute the tier1 tests with such a JDK?

I've tried make JDK_UNDER_TEST=build/linux-x64-slowdebug/images/static-jdk/ test TEST=tier1 but it complains about JDK_UNDER_TEST not being a non-control variable.

Hi @jdksjolen, to run jtreg tests on a static-jdk binary, you can set JDK_FOR_COMPILE=<static_jdk>, JDK_FOR_COMPILE=<regular-jdk-path> and extra-problem-lists options in jtreg command line. The GHA testing on static JDK setup those to run tier1 tests:

jdk/.github/workflows/test.yml

Line 185 in aa36799

if [[ '${{ inputs.static-suffix }}' == '-static' ]]; then

https://bugs.openjdk.org/browse/JDK-8303796?focusedId=14745789&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14745789 has a command line example for running tier1 tests on static-jdk.

To build a static JDK: make static-jdk-image.

Hope those are helpful.

jianglizhou · 2025-09-18T00:12:20Z

I had code in an assert which needed to run regardless if in a debug build or not. Why didn't all release builds fail? No clue.

The GHA teir1 testing on static JDK is only enabled for release builds currently. We haven't setup testing on debug builds for static JDK due to resource limit in GHA build environment. @magicus and the build team have solution(s) to address the issue with https://bugs.openjdk.org/browse/JDK-8350450.

afshin-zafari · 2025-09-18T08:22:36Z

The changes in VMATree::copy_into of this PR, are only seen if we test/check the allocation_sites after a baseline. As I checked the gTest/JTREG tests, none of them check the sites that is written out to the report. So, the code for copy_into is never run in release builds and no allocation_site will be stored in a baseline nor written out in the report. No test will check it to discover it.

jdksjolen · 2025-09-18T09:07:39Z

Hi @jianglizhou ,
There's a test failure that only appears in linux-x64-static tier1. I'm looking at running the tests under the static-jdk. So far, I've produced a static-jdk via make CONF=slow static-jdk-image. Do you happen to know how to execute the tier1 tests with such a JDK?
I've tried make JDK_UNDER_TEST=build/linux-x64-slowdebug/images/static-jdk/ test TEST=tier1 but it complains about JDK_UNDER_TEST not being a non-control variable.

Hi @jdksjolen, to run jtreg tests on a static-jdk binary, you can set JDK_FOR_COMPILE=<static_jdk>, JDK_FOR_COMPILE=<regular-jdk-path> and extra-problem-lists options in jtreg command line. The GHA testing on static JDK setup those to run tier1 tests:

jdk/.github/workflows/test.yml

Line 185 in aa36799

if [[ '${{ inputs.static-suffix }}' == '-static' ]]; then

https://bugs.openjdk.org/browse/JDK-8303796?focusedId=14745789&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14745789 has a command line example for running tier1 tests on static-jdk.

To build a static JDK: make static-jdk-image.

Hope those are helpful.

Thank you for your help!

I did something akin to this, and it worked:

$ make CONF=linux-x64 static-jdk-image
$ make CONF=linux-x64 jdk-image
$ make CONF=linux-x64 JDK_FOR_COMPILE='/home/jsjolen/jdk/build/linux-x64/images/jdk/' JDK_UNDER_TEST=/home/jsjolen/jdk/build/linux-x64/images/static-jdk/ test TEST='jtreg:runtime/NMT/ThreadedVirtualAllocTestType.java'

This did reproduce the test failure on linux-x64-static :-).

The changes in VMATree::copy_into of this PR, are only seen if we test/check the allocation_sites after a baseline. As I checked the gTest/JTREG tests, none of them check the sites that is written out to the report. So, the code for copy_into is never run in release builds and no allocation_site will be stored in a baseline nor written out in the report. No test will check it to discover it.

The test ThreadedVirtualAllocTestType does run NMT under detailed mode and was tested in release for all platforms. This was the one that failed in linux-x64-static. The weird thing is that the test succeeded in all other release platforms, even though they should have had the same assert problem.

afshin-zafari

Good that you found the problem. Thanks.

jdksjolen · 2025-09-18T12:25:23Z

/integrate

Thanks!

openjdk · 2025-09-18T12:26:51Z

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

5db1dfe: 8361950: Update to use jtreg 8
a49856b: 8367969: C2: compiler/vectorapi/TestVectorMathLib.java fails without UnlockDiagnosticVMOptions

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-18T12:27:01Z

@jdksjolen Pushed as commit feaa654.

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

jdksjolen added 20 commits August 29, 2025 18:46

Use a copied VMATree instead of a LinkedList

206b36d

Remove now unused iterator

958c970

Static assert copy-constructible

a125e58

Rewrite copying entirely

2e7bd8c

Fix the NodeType*

17d926a

Fix

69c65fe

It's const

7deb9a1

Rename

7c3f1a4

Wait, the VMATree is wrong?

ffaa2da

Aha, missing include

1cceebf

Aha, _root is typed as an IntrusiveRBNode

d44d860

Some more ugly casts

284ce05

Yup

04b5a99

After a lot of casting

76ec83f

Rename to _vma_allocations

ac24b5e

Merge remote-tracking branch 'openjdk/master' into cleanup-membaseline

82df750

Move into rbTree.inline.hpp file

615598e

Missed const-ness

3bfe859

Merge remote-tracking branch 'openjdk/master' into cleanup-membaseline

e93ad21

Delete unneeded copy ctr

9539fe9

openjdk bot changed the title ~~8367249~~ 8367249: [REDO] MemBaseline accesses VMT without using lock Sep 9, 2025

openjdk bot added hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review labels Sep 9, 2025

afshin-zafari approved these changes Sep 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2025

caspernorrbin approved these changes Sep 9, 2025

View reviewed changes

Merge remote-tracking branch 'openjdk/master' into cleanup-membaseline

c6c65a8

openjdk bot removed the ready Pull request is ready to be integrated label Sep 15, 2025

jdksjolen added 2 commits September 16, 2025 11:51

Fix the rest of the incorrect instances

41af4ea

const-ify a bit

032fc80

How did this even work in any release build?

c07e5cc

Merge remote-tracking branch 'openjdk/master' into cleanup-membaseline

993416d

afshin-zafari approved these changes Sep 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 18, 2025

openjdk bot closed this Sep 18, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 18, 2025

8367249: [REDO] MemBaseline accesses VMT without using lock #27170

8367249: [REDO] MemBaseline accesses VMT without using lock #27170

Uh oh!

Conversation

jdksjolen commented Sep 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 9, 2025

Uh oh!

openjdk bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 9, 2025

Uh oh!

afshin-zafari left a comment

Choose a reason for hiding this comment

Uh oh!

caspernorrbin left a comment

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jdksjolen commented Sep 10, 2025

Uh oh!

dholmes-ora commented Sep 11, 2025

Uh oh!

jdksjolen commented Sep 11, 2025

Uh oh!

jdksjolen commented Sep 16, 2025

Uh oh!

afshin-zafari commented Sep 16, 2025

Uh oh!

jdksjolen commented Sep 17, 2025

Uh oh!

jdksjolen commented Sep 17, 2025

Uh oh!

jdksjolen commented Sep 17, 2025

Uh oh!

jianglizhou commented Sep 18, 2025

Uh oh!

jianglizhou commented Sep 18, 2025

Uh oh!

afshin-zafari commented Sep 18, 2025

Uh oh!

jdksjolen commented Sep 18, 2025

Uh oh!

afshin-zafari left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen commented Sep 18, 2025

Uh oh!

openjdk bot commented Sep 18, 2025

Uh oh!

openjdk bot commented Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

jdksjolen commented Sep 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 9, 2025 •

edited

Loading

mlbridge bot commented Sep 9, 2025 •

edited

Loading