8352696: JFR: assert(false): EA: missing memory path #24192

mgronlun · 2025-03-24T11:35:03Z

Greetings,

In debug builds, we can sometimes assert because C2's Escape Analysis does not recognize a pattern where one input of memory Phi node is a MergeMem node, and another is a RAW store.

This pattern is created by the jdk.jfr.internal.JVM.commit() intrinsic, which is inlined because of inlining JFR events, for example jdk.VirtualThreadStart.

As a result, EA complains about a strange memory graph.

Testing: jdk_jfr

Thanks
Markus

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-8352696: JFR: assert(false): EA: missing memory path (Bug - P3)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to c63fb608
Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24192

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-24T11:35:33Z

👋 Welcome back mgronlun! 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-03-24T11:35:44Z

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

8352696: JFR: assert(false): EA: missing memory path

Reviewed-by: thartmann, shade, kvn

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

17dc30c: 8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead
6bc4803: 8351277: Remove pipewire from AIX build
c94bc74: 8352595: Regression of JDK-8314999 in IR matching
48fac66: 8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
99c8a6e: 8350463: AArch64: Add vector rearrange support for small lane count vectors
b2da0d3: 8352289: [macos] Review skipped tests in tools/jpackage/macosx/SigningPackage*
ba658a7: 8349522: AArch64: Add backend implementation for new unsigned and saturating vector operations
5625b43: 8350429: runtime/NMT/CheckForProperDetailStackTrace.java should only run for debug JVM
2c60fc5: 8352176: Automate setting up environment for mac signing tests
6e6a39d: 8347321: [ubsan] CGGlyphImages.m:553:30: runtime error: nan is outside the range of representable values of type 'unsigned long'
... and 9 more: https://git.openjdk.org/jdk/compare/a8757332667df3fe41a29a7eedb2a7234d23c2a0...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 · 2025-03-24T11:36:28Z

@mgronlun The following labels will be automatically applied to this pull request:

graal
hotspot

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.

mgronlun · 2025-03-24T11:36:59Z

/remove hotspot

mgronlun · 2025-03-24T11:37:34Z

/label remove hotspot

mgronlun · 2025-03-24T11:37:52Z

/label remove graal

openjdk · 2025-03-24T11:38:02Z

@mgronlun Unknown command remove - for a list of valid commands use /help.

openjdk · 2025-03-24T11:38:10Z

@mgronlun
The hotspot label was successfully removed.

openjdk · 2025-03-24T11:38:46Z

@mgronlun
The graal label was successfully removed.

mgronlun · 2025-03-24T11:38:56Z

/label add hotspot-jfr

mgronlun · 2025-03-24T11:39:10Z

/label add hotspot-compiler

openjdk · 2025-03-24T11:39:22Z

@mgronlun
The hotspot-jfr label was successfully added.

openjdk · 2025-03-24T11:39:48Z

@mgronlun
The hotspot-compiler label was successfully added.

mlbridge · 2025-03-24T11:44:47Z

Webrevs

test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java

TobiHartmann

Looks good to me otherwise.

TobiHartmann · 2025-03-24T12:03:21Z

test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java

+ * @requires vm.flagless
+ * @requires vm.hasJFR & vm.debug
+ * @library /test/lib /test/jdk
+ * @run main/othervm jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA


Maybe use -Xbatch?

What would it give?

Disabling background compilation makes it more deterministic?

Yes and you might need fewer runs to trigger compilation and wait for it to finish.

test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java

mgronlun · 2025-03-24T12:18:49Z

Looks good to me otherwise.

Thanks so much for taking a look @TobiHartmann !

src/hotspot/share/opto/library_call.cpp

vnkozlov

Looks good.

TobiHartmann

Looks good.

mgronlun · 2025-03-25T10:36:36Z

Thank you @TobiHartmann, @shipilev and @vnkozlov for your reviews!

mgronlun · 2025-03-25T10:36:58Z

/integrate

openjdk · 2025-03-25T10:37:29Z

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

c002b97: 8352676: Opensource JMenu tests - series1
bdcac98: 8347459: C2: missing transformation for chain of shifts/multiplications by constants
3d3b782: 8352607: RISC-V: use cmove in min/max when Zicond is supported
9f582e5: 8320997: RISC-V: C2 ReverseV
6879c44: 8351405: G1: Collection set early pruning causes suboptimal region selection
aee4d69: 8348829: Remove ObjectMonitor perf counters
f9bcef4: 8351627: C2 AArch64 ROR/ROL: assert((1 << ((T>>1)+3)) > shift) failed: Invalid Shift value
17dc30c: 8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead
6bc4803: 8351277: Remove pipewire from AIX build
c94bc74: 8352595: Regression of JDK-8314999 in IR matching
... and 16 more: https://git.openjdk.org/jdk/compare/a8757332667df3fe41a29a7eedb2a7234d23c2a0...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-25T10:37:44Z

@mgronlun Pushed as commit 721ef76.

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

openjdk bot added the rfr Pull request is ready for review label Mar 24, 2025

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Mar 24, 2025

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Mar 24, 2025

8352696

8e4185c

openjdk bot removed the graal graal-dev@openjdk.org label Mar 24, 2025

openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Mar 24, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 24, 2025

TobiHartmann reviewed Mar 24, 2025

View reviewed changes

test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java Outdated Show resolved Hide resolved

TobiHartmann approved these changes Mar 24, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Mar 24, 2025

mgronlun added 2 commits March 24, 2025 13:22

indentation

cd7f30b

merged requires

e072153

shipilev reviewed Mar 24, 2025

View reviewed changes

src/hotspot/share/opto/library_call.cpp Outdated Show resolved Hide resolved

mgronlun added 2 commits March 24, 2025 13:37

Xbatch

0ba8abf

fold

c63fb60

vnkozlov approved these changes Mar 24, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Mar 24, 2025

simplified test

cae054c

mgronlun requested review from TobiHartmann, shipilev and vnkozlov March 24, 2025 19:26

shipilev approved these changes Mar 25, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 25, 2025

TobiHartmann approved these changes Mar 25, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 25, 2025

openjdk bot closed this Mar 25, 2025

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

8352696: JFR: assert(false): EA: missing memory path #24192

8352696: JFR: assert(false): EA: missing memory path #24192

Uh oh!

Conversation

mgronlun commented Mar 24, 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 Mar 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

mgronlun Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

mgronlun Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgronlun commented Mar 24, 2025

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

mgronlun commented Mar 25, 2025

Uh oh!

mgronlun commented Mar 25, 2025

Uh oh!

openjdk bot commented Mar 25, 2025

Uh oh!

openjdk bot commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

mgronlun commented Mar 24, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 24, 2025 •

edited

Loading

mlbridge bot commented Mar 24, 2025 •

edited

Loading