On Thu, 19 Dec 2024 00:12:52 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: removed unneeded check for JvmtiExport::can_post_frame_pop()
>
> src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 261:
>
>> 259: Thread *current = Thread::current();
>> 260: #endif
>> 261: assert(get_thread() == nullptr ||
>> get_thread()->is_handshake_safe_for(current),
>
> Suggestion:
>
> assert(get_thread() == nullptr ||
> get_thread()->is_handshake_safe_for(Thread::current()),
Thank you for the comment. Chris suggested the same.
I've fixed all such places in the file to make it consistent.
> test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp
> line 44:
>
>> 42: static
>> 43: bool isTestThread(JNIEnv *jni, jvmtiEnv *jvmti, jthread thr) {
>> 44: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr);
>
> Only thread name is required, `get_thread_name` can be used
Thanks. Updated now.
> test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp
> line 55:
>
>> 53: jclass cls;
>> 54: char *mname, *msig, *csig;
>> 55: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr);
>
> Only thread name is required, `get_thread_name` can be used
Thank. Updated now.
> test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp
> line 81:
>
>> 79: jclass cls;
>> 80: char *csig;
>> 81: jvmtiThreadInfo inf = get_thread_info(jvmti, jni, thr);
>
> Looks like this is not needed (and inf.name not deallocated)
Good catch. Removed the line now.
> test/hotspot/jtreg/serviceability/jvmti/events/FramePop/ClearAllFramePops/libClearAllFramePops.cpp
> line 137:
>
>> 135: LOG("(GetCapabilities) unexpected error: %s (%d)\n",
>> TranslateError(err), err);
>> 136: return JNI_ERR;
>> 137: }
>
> I don't think this is needed
Thanks. Removed now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1891157836
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1891161525
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1891162238
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1891159383
PR Review Comment: https://git.openjdk.org/jdk/pull/22744#discussion_r1891160447
