8368002: [lworld] Crash in ThawBase::remove_top_compiled_frame_from_chunk #1603
Conversation
|
👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into |
|
@pchilano 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: 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 435 new commits pushed to the
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 (@coleenp) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
bot
added
ready
Webrevs
|
| } | ||
|
|
||
| int ThawBase::remove_scalarized_frames(StackChunkFrameStream<ChunkFrames::CompiledOnly>& f, stackChunkOop chunk, int &argsize) { | ||
| int ThawBase::remove_scalarized_frames(StackChunkFrameStream<ChunkFrames::CompiledOnly>& f, stackChunkOop chunk, int frames_size, int &argsize) { |
There was a problem hiding this comment.
Thanks for explaining this change to me. I think this parameter is confusing. It's the incoming frame_size from the stub frame, and this adds in all the frame sizes from the frames that need a stack repair.
It might make more sense to me to distinguish these things, have the parameter be "stub_frame_size" or "top_frame_size" and to have a local to accumulate the visited frames sizes like:
int frames_size = stub_frame_size;
There was a problem hiding this comment.
I removed that parameter altogether. I realized we can remove all the duplicated code in remove_scalarized_frames(), so that the method just returns the size of all the frames removed. Then for the stub case, we add this value to the size of the stub frame. For the non-stub case, this returned value is the final size.
| } | ||
|
|
||
| int ThawBase::remove_scalarized_frames(StackChunkFrameStream<ChunkFrames::CompiledOnly>& f, stackChunkOop chunk, int &argsize) { | ||
| DEBUG_ONLY(intptr_t* const chunk_sp = chunk->start_address() + chunk->sp();) |
There was a problem hiding this comment.
Can you remove the 'chunk' parameter too?
There was a problem hiding this comment.
Right, also not needed. Removed.
|
Thanks Coleen! |
|
/sponsor |
|
Going to push as commit feee12a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
2 participants
Please review this small fix. When thawing in the fast path, the top frame could be a runtime stub due to preempting on monitorenter. In the changes for JDK-8336845 I missed this, leading to a crash when dereferencing the nullptr returned by
f.cb()->as_nmethod_or_null()inThawBase::remove_top_compiled_frame_from_chunk.I was able to reproduce the failure locally and verified it is now fixed. I did run into a pre-existing crash with Jetty (filed JDK-8368099). I also run all tests in java/lang/Thread/virtual stressing this path, tests Fuzz.java and TestVirtualThreads.java, plus extra mach5 tier testing.
Thanks,
Patricio
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1603/head:pull/1603$ git checkout pull/1603Update a local copy of the PR:
$ git checkout pull/1603$ git pull https://git.openjdk.org/valhalla.git pull/1603/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1603View PR using the GUI difftool:
$ git pr show -t 1603Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1603.diff
Using Webrev
Link to Webrev Comment