On Thu, 21 Aug 2025 16:01:41 GMT, Leonid Mesnik <[email protected]> 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
