8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result' #22361

MBaesken · 2024-11-25T13:56:20Z

Seems we miss initialization of _last_freeze_fail_result in the JavaThread constructor, this should be added.
Causes otherwise ubsan issues in the test java/lang/Thread/virtual/MonitorEnterExit.java#Xcomp-TieredStopAtLevel1-LM_LEGACY

/priv/jenkins/client-home/workspace/openjdk-jdk-weekly-linux_x86_64-opt/jdk/src/hotspot/share/runtime/javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result'
#0 0x7f5edef378eb in JavaThread::last_freeze_fail_result() src/hotspot/share/runtime/javaThread.hpp:1241
#1 0x7f5edef378eb in JVM_VirtualThreadPinnedEvent src/hotspot/share/prims/jvm.cpp:3805

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-8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result' (Bug - P4)

Reviewers

Richard Reingruber (@reinrich - Reviewer)
Patricio Chilano Mateo (@pchilano - Reviewer)

Contributors

Richard Reingruber <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22361

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-25T13:57:17Z

👋 Welcome back mbaesken! 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 · 2024-11-25T13:58:06Z

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

8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result'

Co-authored-by: Richard Reingruber <[email protected]>
Reviewed-by: rrich, pchilanomate

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

4c33caa: 8344609: Check ResourceMark nesting when allocating a GrowableArray on an alternative ResourceArea
4b92816: 8345375: Improve debuggability of test/jdk/java/net/Socket/CloseAvailable.java
e15912b: 8345248: Module name 'transitive' not accepted for requires transitive
521ed72: 8345357: test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java fails in ubuntu22.04
447f8d4: 8345353: Test for JDK-8344800 W3C DTDs and XSDs in the built-in Catalog
43b337e: 8344304: [s390x] ubsan: negation of -2147483648 cannot be represented in type 'int'
7ec36bb: 8343001: Adjust XSLT and XPath Extension Function Property
c143138: 8345351: RISC-V: Rename macro-assembler routine cmpxchg_weak to weak_cmpxchg
4237897: 8345341: Fix incorrect log message in JDI stop002t test
82e8aa6: 8345415: Rollback JDK-8301991 change on xmlsecurity_de.properties
... and 173 more: https://git.openjdk.org/jdk/compare/e29b0edc9739801c8371ac601a466428916d60b4...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 · 2024-11-25T13:58:45Z

@MBaesken 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 · 2024-11-25T14:01:24Z

Webrevs

reinrich · 2024-11-25T17:06:57Z

Hi @MBaesken

I don't think this fix will help.
The uninitialized field is read in JVM_VirtualThreadPinnedEvent.
With your change freeze_ok will be read and the assertion in the next line will fail.

The real problem seems to be that JavaThread::_last_freeze_fail_result is not always set when the virtual thread is pinned because the freeze attempt failed.

Currently the setting is only ever done in freeze_epilog but freeze_epilog is not always reached from freeze_internal.

From looking quickly I think there are (at least) 2 locations where setting JavaThread::_last_freeze_fail_result is missed:

res is set and returned a few lines below without setting _last_freeze_fail_result
preempt_epilog also fails to set _last_freeze_fail_result

My suggestion would be to a add setting _last_freeze_fail_result in these cases too. Could look like this 723c1c1.

dholmes-ora

The missing initializer is fine but note that as @reinrich stated in the JBS issue there is actually a bug here as we should have already set a reason if we are calling JVM_VirtualThreadPinnedEvent. With your change we should now get an assertion failure.

dholmes-ora · 2024-11-25T21:34:02Z

EDIT: for some reason @reinrich 's comment was not showing up when I made mine.

MBaesken · 2024-11-26T09:06:35Z

okay I can add setting the value with set_last_freeze_fail_result at the locations proposed.
Btw should we have another value e.g. freeze_initialized for the constructor ? I took freeze_ok but maybe this is not representing well the init state ?

reinrich · 2024-11-26T10:19:59Z

Btw should we have another value e.g. freeze_initialized for the constructor ? I took freeze_ok but maybe this is not representing well the init state ?

I don't think that's needed. Nice thing about initializing with freeze_ok is that assertions already check that the value is not equal to freeze_ok when it is read (see comment above). So better revert to initializing with freeze_ok.

It is easy to forget setting _last_freeze_fail_result therefore the callers of freeze_internal should assert that it has been set. I'd suggest to do it as in this commit: 99e6240

dholmes-ora

Seems reasonable but I'm by no means a freeze/thaw expert. I'd really like @pchilano to review this but he won't be back until Monday 2nd December.

Thanks

src/hotspot/share/runtime/continuationFreezeThaw.cpp

MBaesken · 2024-11-27T08:08:52Z

/contributor add @reinrich

openjdk · 2024-11-27T08:09:28Z

@MBaesken
Contributor Richard Reingruber <[email protected]> successfully added.

reinrich

Looks good.
Thanks, Richard.

reinrich · 2024-12-02T16:12:36Z

@pchilano whould you mind reviewing this small fix?
While it is true that JavaThread::_last_freeze_fail_result is not initialized the real problem is that it is not always set when freezing fails. This is fixed and 2 new assertions make sure that result of freeze_internal was stored to JavaThread::_last_freeze_fail_result.
Might be even better to move setting JavaThread::_last_freeze_fail_result up in the call hierarchy and replace the assertions we've added. Let us know what you think.

pchilano

Looks good, thanks for fixing it.

pchilano · 2024-12-02T17:04:58Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

  if (UNLIKELY(res != freeze_ok)) {
+    JFR_ONLY(current->set_last_freeze_fail_result(res);)


Note that we don’t need to save the result when unmounting from the VM, i.e the preempt case. We return it from Continuation::try_preempt() so it is already available when we call post_vthread_pinned_event() [1][2]. When unmounting from Java we do need to save it because the call to post_vthread_pinned_event() will be done later in VirtualThread.postPinnedEvent(), where the return value from the freeze call is not available anymore.

[1]

jdk/src/hotspot/share/runtime/objectMonitor.cpp

Line 603 in 30b8bbe

current->post_vthread_pinned_event(&vthread_pinned_event, "Contended monitor enter", result);

[2]

jdk/src/hotspot/share/runtime/objectMonitor.cpp

Line 1824 in 30b8bbe

current->post_vthread_pinned_event(&vthread_pinned_event, "Object.wait", result);

I see. Thanks for making us aware.

This is the complete diff when everything related to the preempt case is reverted again: reinrich/jdk@e29b0ed...ce3ddf6

@MBaesken could you please check and apply this commit to do the reverting reinrich@ce3ddf6 if you're ok with it?

The JFR event support changed a bit near the end so looks like this slipped through. For the non-preempt cases, the _last_freeze_fail_XXX values must be set so that the later call to record an event has the last reason/time. So I think the change look good. I see the failure was in legacy mode so more likely to run into this.

MBaesken · 2024-12-03T09:19:08Z

Adjusted as suggested.

reinrich

Looks good (test results too).
Thanks, Richard.

MBaesken · 2024-12-04T08:10:59Z

Thanks for the reviews !

/integrate

openjdk · 2024-12-04T08:11:37Z

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

943aa03: 8345404: [ppc64le] ProblemList TestAllocOutOfMemory.java#large
9e2b66f: 8345178: RISC-V: Add gtests for narrow cmpxchg
4c33caa: 8344609: Check ResourceMark nesting when allocating a GrowableArray on an alternative ResourceArea
4b92816: 8345375: Improve debuggability of test/jdk/java/net/Socket/CloseAvailable.java
e15912b: 8345248: Module name 'transitive' not accepted for requires transitive
521ed72: 8345357: test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java fails in ubuntu22.04
447f8d4: 8345353: Test for JDK-8344800 W3C DTDs and XSDs in the built-in Catalog
43b337e: 8344304: [s390x] ubsan: negation of -2147483648 cannot be represented in type 'int'
7ec36bb: 8343001: Adjust XSLT and XPath Extension Function Property
c143138: 8345351: RISC-V: Rename macro-assembler routine cmpxchg_weak to weak_cmpxchg
... and 175 more: https://git.openjdk.org/jdk/compare/e29b0edc9739801c8371ac601a466428916d60b4...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-04T08:11:44Z

@MBaesken Pushed as commit cf1eb58.

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

MBaesken · 2024-12-04T08:14:01Z

Thanks for the reviews !

/integrate

openjdk · 2024-12-04T08:14:22Z

@MBaesken The command integrate can only be used in open pull requests.

JDK-8344935

c70c580

openjdk bot added the rfr Pull request is ready for review label Nov 25, 2024

openjdk bot added the hotspot-runtime [email protected] label Nov 25, 2024

dholmes-ora reviewed Nov 25, 2024

View reviewed changes

set last_freeze at more code locations

f8044bc

remove added state, add assert

a64088d

dholmes-ora reviewed Nov 27, 2024

View reviewed changes

src/hotspot/share/runtime/continuationFreezeThaw.cpp Outdated Show resolved Hide resolved

reinrich reviewed Nov 27, 2024

View reviewed changes

src/hotspot/share/runtime/continuationFreezeThaw.cpp Outdated Show resolved Hide resolved

reinrich reviewed Nov 27, 2024

View reviewed changes

src/hotspot/share/runtime/continuationFreezeThaw.cpp Outdated Show resolved Hide resolved

add parenthesis, rename variables

a0f9b70

reinrich approved these changes Nov 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 28, 2024

pchilano reviewed Dec 2, 2024

View reviewed changes

pchilano comments

6c33887

openjdk bot removed the ready Pull request is ready to be integrated label Dec 3, 2024

MBaesken added 2 commits December 3, 2024 10:09

add missing if check

568d7af

freeze_preempt remove JFR_ONLY coding

c0df96a

pchilano approved these changes Dec 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2024

reinrich approved these changes Dec 4, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2024

openjdk bot closed this Dec 4, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 4, 2024

		if (UNLIKELY(res != freeze_ok)) {
		JFR_ONLY(current->set_last_freeze_fail_result(res);)

8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result' #22361

8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result' #22361

Uh oh!

Conversation

MBaesken commented Nov 25, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Nov 25, 2024

Uh oh!

openjdk bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 25, 2024

Uh oh!

mlbridge bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

reinrich commented Nov 25, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Nov 25, 2024

Uh oh!

MBaesken commented Nov 26, 2024

Uh oh!

reinrich commented Nov 26, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MBaesken commented Nov 27, 2024

Uh oh!

openjdk bot commented Nov 27, 2024

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

reinrich commented Dec 2, 2024

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

pchilano Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

reinrich Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

reinrich Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

AlanBateman Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Dec 3, 2024

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

MBaesken commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

Uh oh!

MBaesken commented Nov 25, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 25, 2024 •

edited

Loading

mlbridge bot commented Nov 25, 2024 •

edited

Loading