RFR: 8365937: post_method_exit might incorrectly set was_popped_by_exception and value in the middle of stack unwinding [v2]

[David Holmes]

On Thu, 21 Aug 2025 16:01:41 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> The void `JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame current_frame) `calculates
>>   `bool exception_exit = state->is_exception_detected() && !state->is_exception_caught();`
>> to find if method exit normally or by exception.
>> However, JvmtiExport::post_method_exit( method is not called at all in the case of exception. See
>> `void JvmtiExport::notice_unwind_due_to_exception(JavaThread *thread, Method* method, address location, oop exception, bool in_handler_frame)`
>> where post_method_exit_inner is called directly.
>> 
>> The `exception_exit` is true when exception is processed and the current method is called in the middle of stack unwinding. 
>> 
>> 
>> The fix was a part of
>> https://github.com/openjdk/jdk/pull/26713
>> for
>> https://bugs.openjdk.org/browse/JDK-8365192
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   assertion added.

Looks good. A few style nits and a couple of suggestions.

This will make the other PR much easier to review.

Thanks

src/hotspot/share/prims/jvmtiExport.cpp line 1843:

> 1841:   // return a flag when a method terminates by throwing an exception
> 1842:   // i.e. if an exception is thrown and it's not caught by the current method
> 1843:   bool exception_exit = state->is_exception_detected() && !state->is_exception_caught();

Can we assert this is not an exception exit please.

src/hotspot/share/prims/jvmtiExport.cpp line 1851:

> 1849:     BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
> 1850:     assert(type == T_VOID || current_frame.interpreter_frame_expression_stack_size() > 0,
> 1851:       "Stack shouldn't be empty");

Suggestion:

    assert(type == T_VOID || current_frame.interpreter_frame_expression_stack_size() > 0,
           "Stack shouldn't be empty");

src/hotspot/share/prims/jvmtiExport.cpp line 1864:

> 1862:   // Deferred transition to VM, so we can stash away the return oop before GC
> 1863:   // Note that this transition is not needed when throwing an exception, because
> 1864:   // there is no oop to retain.

The comment on lines 1863-1864 is no longer needed.

src/hotspot/share/prims/jvmtiExport.cpp line 1867:

> 1865:   JavaThread* current = thread; // for JRT_BLOCK
> 1866:   JRT_BLOCK
> 1867:     post_method_exit_inner(thread, mh, state, false, current_frame, value);

Suggestion:

    post_method_exit_inner(thread, mh, state, false /* not exception exit */, current_frame, value);

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/TestMethodExitWithPendingException.java line 27:

> 25:  * @test
> 26:  * @summary Test verifies that MethodExit event is correctly posted
> 27:  * if method is called while there is a pending exception on this thread.

Suggestion:

 * @summary Test verifies that MethodExit event is correctly posted
 *          if method is called while there is a pending exception on this thread.

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 33:

> 31: // This method exit callback actually works only for 2 methods:
> 32: // 1) for ExceptionExit it verifies that method exit
> 33: //    has been popped by exception and call 'upCall' mthod using JNI.

Suggestion:

//    has been popped by exception and calls 'upCall' method using JNI.

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 40:

> 38: cbMethodExit(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread, jmethodID method,
> 39:              jboolean was_popped_by_exception, jvalue return_value) {
> 40:   const char * mname = get_method_name(jvmti, jni, method);

Style nit: please bind the `*` to the type in all pointer declarations e.g. `char* mname` not `char * mname` or `char *mname`.  At present all three variants exist in the code. Thanks

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 67:

> 65:   }
> 66:   jmethodID upcall_method = jni->GetStaticMethodID(main_class,
> 67:       "upCall", "()Ljava/lang/String;");

Suggestion:

  jmethodID upcall_method = jni->GetStaticMethodID(main_class,
                                                   "upCall", "()Ljava/lang/String;");

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 104:

> 102:   check_jvmti_error(err, "SetEventCallbacks");
> 103:   jvmti_env = jvmti;
> 104:  return JNI_OK;

Suggestion:

  jvmti_env = jvmti;
  return JNI_OK;

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 109:

> 107: 
> 108: extern "C" {
> 109: JNIEXPORT void JNICALL

Suggestion:

extern "C" {

JNIEXPORT void JNICALL

test/hotspot/jtreg/serviceability/jvmti/events/MethodExit/PendingException/libTestMethodExitWithPendingException.cpp line 125:

> 123: }
> 124: 
> 125: }

Suggestion:

} // extern "C"

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26886#pullrequestreview-3163239887
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306237793
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306236038
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306244212
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306236955
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306247384
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306249362
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306260149
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306252496
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306255064
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306256945
PR Review Comment: https://git.openjdk.org/jdk/pull/26886#discussion_r2306257329