Skip to content

8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state #26713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+6 −2
Open

8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state #26713

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
47a32e8
8365192: post_meth_exit should be in vm state when calling get_jvmti_…
lmesnik Aug 9, 2025
7cce73e
simplified after feedback
lmesnik Aug 13, 2025
977c9eb
fixed name
lmesnik Aug 13, 2025
31046ba
wong phase
lmesnik Aug 13, 2025
255c0ba
added _
lmesnik Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/hotspot/share/prims/jvmtiExport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,7 @@ JvmtiExport::get_jvmti_interface(JavaVM *jvm, void **penv, jint version) {
JvmtiThreadState*
JvmtiExport::get_jvmti_thread_state(JavaThread *thread, bool allow_suspend) {
assert(thread == JavaThread::current(), "must be current thread");
assert(thread->thread_state() == _thread_in_vm, "thread should be in vm");
if (thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr) {
JvmtiEventController::thread_started(thread);
if (allow_suspend && thread->is_suspended()) {
Expand Down Expand Up @@ -1831,8 +1832,11 @@ void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame cur
HandleMark hm(thread);
methodHandle mh(thread, method);

JvmtiThreadState *state = get_jvmti_thread_state(thread);

JvmtiThreadState *state = nullptr;
{
ThreadInVMfromJava __tiv(thread);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe rename: __tiv => tiv. The prefix __ is normally used in macros to avoid potential naming conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update on this!

state = get_jvmti_thread_state(thread);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue I see is that get_jvmti_thread_state() can safepoint for virtual threads (and now also for platform threads because of ~ThreadInVMfromJava), which brings us back to the bug 8255452 was trying to fix: if there is a return oop at the top of the stack, it could become invalid if a GC occurs. I think we will have to unconditionally save the return value in case it's an oop, before doing anything else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Patricio! Good catch and suggestion.

Copy link
Contributor

@sspitsyn sspitsyn Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be kind of intrusive, something like below (without changes from Leonid):

@@ -1830,6 +1830,16 @@ void JvmtiExport::post_method_entry(JavaThread *thread, Method* method, frame cu
 void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame current_frame) {
   HandleMark hm(thread);
   methodHandle mh(thread, method);
+  Handle result;
+  jvalue value;
+  oop oop_result;
+  BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
+
+  value.j = 0L;
+  if (is_reference_type(type)) {
+    result = Handle(thread, oop_result);
+    value.l = JNIHandles::make_local(thread, result());
+  }
 
   JvmtiThreadState *state = get_jvmti_thread_state(thread);
 
@@ -1841,22 +1851,14 @@ void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame cur
   // return a flag when a method terminates by throwing an exception
   // i.e. if an exception is thrown and it's not caught by the current method
   bool exception_exit = state->is_exception_detected() && !state->is_exception_caught();
-  Handle result;
-  jvalue value;
-  value.j = 0L;
 
   if (state->is_enabled(JVMTI_EVENT_METHOD_EXIT)) {
     // if the method hasn't been popped because of an exception then we populate
     // the return_value parameter for the callback. At this point we only have
     // the address of a "raw result" and we just call into the interpreter to
     // convert this into a jvalue.
-    if (!exception_exit) {
-      oop oop_result;
-      BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
-      if (is_reference_type(type)) {
-        result = Handle(thread, oop_result);
-        value.l = JNIHandles::make_local(thread, result());
-      }
+    if (exception_exit) {
+      value.j = 0L;
     }
   }

The comments also need to be corrected.
Not sure yet how to make it simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pchilano and @sspitsyn for finding and explaining the issue.
I need some more time to understand how to better correctly preserve the result oop in post_method_exit before we can starting read jvmti_state.

}
if (state == nullptr || !state->is_interp_only_mode()) {
// for any thread that actually wants method exit, interp_only_mode is set
return;
Expand Down