RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Patricio Chilano
patricio.chilano.mateo at oracle.com
Fri Jul 10 05:49:25 UTC 2020
Hi Yasumasa,
On 7/10/20 1:58 AM, Yasumasa Suenaga wrote:
> Hi Patricio,
>
> Thanks for your advice!
> I've fixed testcase as [1], but I still see an error in
> validate-headers-linux-x64-build-1 on submit repo.
> What does it mean? Can you share details?
>
> mach5-one-ysuenaga-JDK-8242428-20200710-0339-12529134
Not your bug, you are getting the header validation failure from
8248570. It was fixed two days ago so just update the repo and it should
work now.
Thanks,
Patricio
> Of course this change can be built on my Linux box (Fedora 32, AMD64,
> GCC 10.1)
>
>
> Yasumasa
>
>
> [1]
> http://hg.openjdk.java.net/jdk/submit/file/45f52142db9d/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>
>
> On 2020/07/10 2:18, Patricio Chilano wrote:
>>
>> On 7/9/20 12:00 PM, Patricio Chilano wrote:
>>> Hi Yasumasa,
>>>
>>> On 7/9/20 9:30 AM, Yasumasa Suenaga wrote:
>>>> On 2020/07/09 17:58, David Holmes wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> On 9/07/2020 10:25 am, Yasumasa Suenaga wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> Thanks for your comment!
>>>>>> I uploaded new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.09/
>>>>>> Diff from previous webrev:
>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/5d167adf8524
>>>>>>
>>>>>> I saw similar build errors in libOneGetThreadListStackTraces.cpp
>>>>>> on Windows.
>>>>>> This webrev fixes them.
>>>>>
>>>>> You shouldn't use %p as it may not be portable. In the VM we use
>>>>> INTPTR_FORMAT and convert the arg using p2i. I don't know what
>>>>> exists in the testing code.
>>>>
>>>> I replaced %p to %lx, and also cast values to unsigned long [1]
>>>> [2], but the test on submit repo was failed.
>>>> Can anyone share details of
>>>> mach5-one-ysuenaga-JDK-8242428-20200709-1030-12500928 ?
>>> These are the errors I see for the macOS build:
>>>
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:14:
>>> error: format specifies type 'long long' but the argument has type
>>> 'jlocation' (aka 'long') [-Werror,-Wformat]
>>> fi1->location, fi2->location);
>>> ^~~~~~~~~~~~~
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:53:29:
>>> error: format specifies type 'long long' but the argument has type
>>> 'jlocation' (aka 'long') [-Werror,-Wformat]
>>> fi1->location, fi2->location);
>>> ^~~~~~~~~~~~~
>>>
>>> These are the ones I see for the Windows build:
>>>
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
>>> warning C4311: 'type cast': pointer truncation from 'jmethodID' to
>>> 'unsigned long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
>>> warning C4302: 'type cast': truncation from 'jmethodID' to 'unsigned
>>> long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
>>> warning C4311: 'type cast': pointer truncation from 'jmethodID' to
>>> 'unsigned long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(48):
>>> warning C4302: 'type cast': truncation from 'jmethodID' to 'unsigned
>>> long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
>>> warning C4311: 'type cast': pointer truncation from 'jthread' to
>>> 'unsigned long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
>>> warning C4302: 'type cast': truncation from 'jthread' to 'unsigned
>>> long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
>>> warning C4311: 'type cast': pointer truncation from 'jthread' to
>>> 'unsigned long'
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp(70):
>>> warning C4302: 'type cast': truncation from 'jthread' to 'unsigned
>>> long'
>>>
>>>
>>> You will probably want to use the macros defined in
>>> src/hotspot/share/utilities/globalDefinitions.hpp. Let me know if
>>> you need me to test something.
>> With these changes the build works okay on Linux, Windows and macOS:
>>
>>
>> ---
>> a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>> +++
>> b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>> @@ -27,4 +27,5 @@
>> #include <stdio.h>
>> #include <stdlib.h>
>> +#include <inttypes.h>
>>
>> #define MAX_FRAMES 100
>> @@ -45,11 +46,11 @@
>> if (fi1->method != fi2->method) { /* jvmtiFrameInfo::method */
>> snprintf(err_msg, sizeof(err_msg),
>> - "method is different: fi1 = %p, fi2 = %p",
>> - fi1->method, fi2->method);
>> + "method is different: fi1 = 0x%016" PRIxPTR " , fi2 =
>> 0x%016" PRIxPTR,
>> + (intptr_t)fi1->method, (intptr_t)fi2->method);
>> env->FatalError(err_msg);
>> } else if (fi1->location != fi2->location) { /*
>> jvmtiFrameInfo::location */
>> snprintf(err_msg, sizeof(err_msg),
>> - "location is different: fi1 = %lld, fi2 = %lld",
>> - fi1->location, fi2->location);
>> + "location is different: fi1 = %" PRId64 " , fi2 = %"
>> PRId64,
>> + (int64_t)fi1->location, (int64_t)fi2->location);
>> env->FatalError(err_msg);
>> }
>> @@ -67,5 +68,5 @@
>> if (!is_same) { /* jvmtiStackInfo::thread */
>> snprintf(err_msg, sizeof(err_msg),
>> - "thread is different: si1 = %p, si2 = %p", si1->thread,
>> si2->thread);
>> + "thread is different: si1 = 0x%016" PRIxPTR " , si2 =
>> 0x%016" PRIxPTR, (intptr_t)si1->thread, (intptr_t)si2->thread);
>> env->FatalError(err_msg);
>> } else if (si1->state != si2->state) { /* jvmtiStackInfo::state */
>>
>>
>> Maybe you can use something like that.
>>
>>
>> Thanks,
>> Patricio
>>> Thanks,
>>> Patricio
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> [1] http://hg.openjdk.java.net/jdk/submit/rev/dfca51958217
>>>> [2] http://hg.openjdk.java.net/jdk/submit/rev/3665361fa91b
>>>>
>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2020/07/09 1:42, Daniel D. Daugherty wrote:
>>>>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
>>>>>>>
>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>> No comments.
>>>>>>>
>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>> L1159: Thread *current_thread = Thread::current();
>>>>>>> Please add "#ifdef ASSERT" above and "#endif" below since
>>>>>>> current_thread is only used for the assert() in this
>>>>>>> function.
>>>>>>>
>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>> L549: jthread java_thread,
>>>>>>> jint max_frame_count)
>>>>>>> L552: _jthread(java_thread),
>>>>>>> Please: s/java_thread/thread/ on both lines.
>>>>>>>
>>>>>>> src/hotspot/share/runtime/vmOperations.hpp
>>>>>>> No comments.
>>>>>>>
>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java
>>>>>>>
>>>>>>> No comments.
>>>>>>>
>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>>>>>>
>>>>>>> L27: #include <errno.h>
>>>>>>> This include is out of order; should be first in the list.
>>>>>>>
>>>>>>> This file doesn't compile on my MBP13:
>>>>>>>
>>>>>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:14:
>>>>>>> error: format specifies type 'unsigned long' but the argument
>>>>>>> has type 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>>>>>>> fi1->method, fi2->method);
>>>>>>> ^~~~~~~~~~~
>>>>>>> ./open/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp:49:27:
>>>>>>> error: format specifies type 'unsigned long' but the argument
>>>>>>> has type 'jmethodID' (aka '_jmethodID *') [-Werror,-Wformat]
>>>>>>> fi1->method, fi2->method);
>>>>>>> ^~~~~~~~~~~
>>>>>>> 2 errors generated.
>>>>>>>
>>>>>>> This change made it compile on my MBP13, but that may break
>>>>>>> it on
>>>>>>> other platforms:
>>>>>>>
>>>>>>> $ hg diff
>>>>>>> diff -r 560847c69fbe
>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>>>>>> ---
>>>>>>> a/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>>>>>> Wed Jul 08 12:13:32 2020 -0400
>>>>>>> +++
>>>>>>> b/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
>>>>>>> Wed Jul 08 12:40:42 2020 -0400
>>>>>>> @@ -46,7 +46,7 @@
>>>>>>> if (fi1->method != fi2->method) { /*
>>>>>>> jvmtiFrameInfo::method */
>>>>>>> snprintf(err_msg, sizeof(err_msg),
>>>>>>> "method is different: fi1 = %lx, fi2 = %lx",
>>>>>>> - fi1->method, fi2->method);
>>>>>>> + (unsigned long) fi1->method, (unsigned long)
>>>>>>> fi2->method);
>>>>>>> env->FatalError(err_msg);
>>>>>>> } else if (fi1->location != fi2->location) { /*
>>>>>>> jvmtiFrameInfo::location */
>>>>>>> snprintf(err_msg, sizeof(err_msg),
>>>>>>>
>>>>>>> I'm not sure of the right platform independent way to output
>>>>>>> the 'method' field.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>> On 7/8/20 4:04 AM, Yasumasa Suenaga wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On 2020/07/08 15:27, David Holmes wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>> On 7/07/2020 6:54 pm, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi David, Serguei,
>>>>>>>>>>
>>>>>>>>>> Serguei, thank you for replying even though you are on vacaiton!
>>>>>>>>>>
>>>>>>>>>> I uploaded new webrev:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.07/
>>>>>>>>>> Diff from previous webrev:
>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/77243b1dcbfe
>>>>>>>>>>
>>>>>>>>>> c'tor of GetSingleStackTraceClosure has jthread argument in
>>>>>>>>>> this webrev.
>>>>>>>>>> Also it does not contain testcase for
>>>>>>>>>> GetThreadListStackTraces with all threads, and
>>>>>>>>>> OneGetThreadListStackTraces would test main thread only.
>>>>>>>>>
>>>>>>>>> All those changes are fine in principle for me. One
>>>>>>>>> nit/suggestion:
>>>>>>>>>
>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>>>>
>>>>>>>>> 544 jthread _java_thread;
>>>>>>>>>
>>>>>>>>> elsewhere "java_thread" refers to a JavaThread, so to avoid
>>>>>>>>> confusion may I suggest this member be named _jthread.
>>>>>>>>
>>>>>>>> I uploaded new webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.08/
>>>>>>>> Diff from previous webrev:
>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/ca6263dbdc87
>>>>>>>>
>>>>>>>>
>>>>>>>>> I'm going to be away for the next couple of days - sorry - but
>>>>>>>>> will try to check email on this if I can.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2020/07/07 15:13, David Holmes wrote:
>>>>>>>>>>> On 7/07/2020 2:57 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>> On 2020/07/07 11:31, David Holmes wrote:
>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hard to keep up with the changes - especially without
>>>>>>>>>>>>> incremental webrevs.
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry, I will upload diff from previous webrev in the next.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> If GetSingleStackTraceClosure also took the jthread as a
>>>>>>>>>>>>> constructor arg, then you wouldn't need to recreate a JNI
>>>>>>>>>>>>> local handle when calling _collector.fill_frames. It's a
>>>>>>>>>>>>> small simplification and not essential at this stage.
>>>>>>>>>>>>
>>>>>>>>>>>> I think we should get jthread from an argument of
>>>>>>>>>>>> do_thread() because do_thread() would pass the thread which
>>>>>>>>>>>> are stopped certainly.
>>>>>>>>>>>> It might be simplification if we pass _calling_thread to
>>>>>>>>>>>> MultipleStackTracesCollector. `jthread` is only needed to
>>>>>>>>>>>> store jvmtiStackInfo.thread . What do you think?
>>>>>>>>>>>
>>>>>>>>>>> I'm not quite sure what you mean.
>>>>>>>>>>>
>>>>>>>>>>> I think there is a bit of a design wart with direct
>>>>>>>>>>> handshakes in that do_thread takes the target JavaThread as
>>>>>>>>>>> an argument. That's useful in a case where you want a
>>>>>>>>>>> HandshakeClosure that can be applied to multiple threads,
>>>>>>>>>>> but that's not typically what is needed with direct
>>>>>>>>>>> handshakes - there is only a single target. With a
>>>>>>>>>>> single-target HandshakeClosure you can capture all the
>>>>>>>>>>> "target" information for the operation in the closure
>>>>>>>>>>> instance. So if the actual do_thread operation wants the
>>>>>>>>>>> jthread corresponding to the target thread then we can store
>>>>>>>>>>> that in the closure rather than recomputing it (you could
>>>>>>>>>>> assert it is the same but that seems overkill to me).
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> For the test ... I don't see how
>>>>>>>>>>>>> Java_GetThreadListStackTraces_checkCallStacks is a valid
>>>>>>>>>>>>> test. It gets the stacks of all live threads, then uses
>>>>>>>>>>>>> that information to use GetThreadListStackTraces to get
>>>>>>>>>>>>> the stack for the same set of threads through a different
>>>>>>>>>>>>> API. It then compares the two sets of stacks for each
>>>>>>>>>>>>> thread expecting them to be the same, but that need only
>>>>>>>>>>>>> be the case for the main thread. Other threads could
>>>>>>>>>>>>> potentially have a different stack (e.g. if this test is
>>>>>>>>>>>>> run with JFR enabled there will be additional threads
>>>>>>>>>>>>> found.) Further I would have expected that there already
>>>>>>>>>>>>> exist tests that check that, for a given thread (which may
>>>>>>>>>>>>> be suspended or known to be blocked) the same stack is
>>>>>>>>>>>>> found through the two different APIs.
>>>>>>>>>>>>
>>>>>>>>>>>> vmTestbase/nsk/jvmti/unit/GetAllStackTraces/getallstktr001
>>>>>>>>>>>> would check all of threads via GetThreadListStackTraces()
>>>>>>>>>>>> and GetAllStackTraces(), so we might be able to remove
>>>>>>>>>>>> GetThreadListStackTraces.java from this webrev.
>>>>>>>>>>>
>>>>>>>>>>> Yes. The existing test only examines a set of test threads
>>>>>>>>>>> that are all blocked on a raw monitor. You do not need to
>>>>>>>>>>> duplicate that test.
>>>>>>>>>>>
>>>>>>>>>>>> OTOH we don't have testcase for GetThreadListStackTraces()
>>>>>>>>>>>> with thread_count == 1, so we need to testcase for it (it
>>>>>>>>>>>> is OneGetThreadListStackTraces.java) It would check whether
>>>>>>>>>>>> the state of target thread is "waiting" before JNI call to
>>>>>>>>>>>> call GetThreadListStackTraces(),
>>>>>>>>>>>
>>>>>>>>>>> Yes we need to test the special cases introduced by your
>>>>>>>>>>> changes - totally agree - and
>>>>>>>>>>> OneGetThreadListStackTraces.java is a good test for that.
>>>>>>>>>>>
>>>>>>>>>>>> and also I expect it would not be run with JFR. (it is not
>>>>>>>>>>>> described @run)
>>>>>>>>>>>
>>>>>>>>>>> The arguments to run with JFR (or a bunch of other things)
>>>>>>>>>>> can be passed to the jtreg test harness to be applied to all
>>>>>>>>>>> tests.
>>>>>>>>>>>
>>>>>>>>>>>> Of course we can check GetThreadListStackTraces() with main
>>>>>>>>>>>> thread, but it is not the test for direct handshake for
>>>>>>>>>>>> other thread.
>>>>>>>>>>>
>>>>>>>>>>> Right - that test already exists as per the above.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> David
>>>>>>>>>>>>> -----
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 6/07/2020 11:29 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think C++ is more simple to implement the test agent as
>>>>>>>>>>>>>> you said.
>>>>>>>>>>>>>> So I implement it in C++ in new webrev. Could you review
>>>>>>>>>>>>>> again?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.06/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also I refactored libGetThreadListStackTraces.cpp, and
>>>>>>>>>>>>>> I've kept exception check after IsSameObject().
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2020/07/06 16:32, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thank you for the update.
>>>>>>>>>>>>>>> I think, a pending exception after IsSameObject needs to
>>>>>>>>>>>>>>> be checked.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The checkStackInfo() needs one more refactoring as I've
>>>>>>>>>>>>>>> already suggested.
>>>>>>>>>>>>>>> The body of the loop at L68-L78 should be converted to a
>>>>>>>>>>>>>>> function check_frame_info.
>>>>>>>>>>>>>>> The si1->frame_buffer[i] and si2->frame_buffer[i] will
>>>>>>>>>>>>>>> be passed as fi1 and fi2.
>>>>>>>>>>>>>>> The index can be passed as well.
>>>>>>>>>>>>>>> I'm still suggesting to simplify the local exception_msg
>>>>>>>>>>>>>>> to something shorter like err_msg or exc_msg.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I'm not sure using fatal is right here:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This fragment looks strange:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 152 if ((*env)->IsSameObject(env,
>>>>>>>>>>>>>>> stack_info[i].thread, thread)) {
>>>>>>>>>>>>>>> 153 target_info = &stack_info[i];
>>>>>>>>>>>>>>> 154 break;
>>>>>>>>>>>>>>> 155 } else if ((*env)->ExceptionOccurred(env)) {
>>>>>>>>>>>>>>> 156 (*env)->ExceptionDescribe(env);
>>>>>>>>>>>>>>> 157 (*env)->FatalError(env, __FILE__);
>>>>>>>>>>>>>>> 158 }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I expected it to be:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> jboolean same = (*env)->IsSameObject(env,
>>>>>>>>>>>>>>> stack_info[i].thread, thread);
>>>>>>>>>>>>>>> if ((*env)->ExceptionOccurred(env)) {
>>>>>>>>>>>>>>> (*env)->ExceptionDescribe(env);
>>>>>>>>>>>>>>> (*env)->FatalError(env, __FILE__);
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> if (same) {
>>>>>>>>>>>>>>> target_info = &stack_info[i];
>>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Would it better to port this agent to C++ to simplify
>>>>>>>>>>>>>>> this code nicer?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 7/5/20 06:13, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>>>>>>> I refactored testcase. Could you review again?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.05/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It would check Java exception after IsSameObject()
>>>>>>>>>>>>>>>> call. Does it need?
>>>>>>>>>>>>>>>> Any exceptions are not described in JNI document[1],
>>>>>>>>>>>>>>>> and JNI implementation (jni_IsSameObject()) does not
>>>>>>>>>>>>>>>> seem to throw it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>> https://docs.oracle.com/en/java/javase/14/docs/specs/jni/functions.html#issameobject
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 2020/07/05 14:46, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Okay, thanks.
>>>>>>>>>>>>>>>>> Then I'm okay to keep the GetSingleStackTraceClosure.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c.html
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c.html
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I'm not sure the function 'is_same_thread() is needed.
>>>>>>>>>>>>>>>>> Why do not use the JNI IsSameObject instead?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> It seems to be a typo at L132 and L137.
>>>>>>>>>>>>>>>>> You, probably. did not want to print the same
>>>>>>>>>>>>>>>>> information for stack_info_1[i].frame_buffer[j].XXX
>>>>>>>>>>>>>>>>> twice.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The code at lines 112-142 is not readable.
>>>>>>>>>>>>>>>>> I'd suggest to make a couple of refactoring steps.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> First step to simplify this a little bit would be with
>>>>>>>>>>>>>>>>> some renaming and getting rid of indexes:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 71 char err_msg[EXCEPTION_MSG_LEN] = {0};
>>>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>>> 112 /* Iterate all jvmtiStackInfo to check */
>>>>>>>>>>>>>>>>> 113 for (i = 0; i < num_threads, *exception_msg !=
>>>>>>>>>>>>>>>>> '\0'; i++) {
>>>>>>>>>>>>>>>>> jvmtiStackInfo *si1 = stack_info_1[i];
>>>>>>>>>>>>>>>>> jvmtiStackInfo *si2 = stack_info_2[i];
>>>>>>>>>>>>>>>>> 114 if (!IsSameObject(env, si1.thread,
>>>>>>>>>>>>>>>>> si2.thread)) { /* jvmtiStackInfo::thread */
>>>>>>>>>>>>>>>>> 115 snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>>>>>> 116 "thread[%d] is different:
>>>>>>>>>>>>>>>>> stack_info_1 = %p, stack_info_2 = %p",
>>>>>>>>>>>>>>>>> 117 i, sinfo1.thread, sinfo2.thread);
>>>>>>>>>>>>>>>>> 118 } else if (si1.state != si2.state) { /*
>>>>>>>>>>>>>>>>> jvmtiStackInfo::state */
>>>>>>>>>>>>>>>>> 119 snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>>>>>> 120 "state[%d] is different:
>>>>>>>>>>>>>>>>> stack_info_1 = %d, stack_info_2 = %d",
>>>>>>>>>>>>>>>>> 121 i, si1.state, si2.state);
>>>>>>>>>>>>>>>>> 122 } else if (si1.frame_count !=
>>>>>>>>>>>>>>>>> si2.frame_count) { /* jvmtiStackInfo::frame_count */
>>>>>>>>>>>>>>>>> 123 snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>>>>>> 124 "frame_count[%d] is different: stack_info_1 =
>>>>>>>>>>>>>>>>> %d, stack_info_2 = %d",
>>>>>>>>>>>>>>>>> 125 i, si1.frame_count,
>>>>>>>>>>>>>>>>> si2.frame_count);
>>>>>>>>>>>>>>>>> 126 } else {
>>>>>>>>>>>>>>>>> 127 /* Iterate all jvmtiFrameInfo to check */
>>>>>>>>>>>>>>>>> 128 for (j = 0; j < si1.frame_count; j++) {
>>>>>>>>>>>>>>>>> 129 if (si1.frame_buffer[j].method !=
>>>>>>>>>>>>>>>>> si1.frame_buffer[j].method) { /*
>>>>>>>>>>>>>>>>> jvmtiFrameInfo::method */
>>>>>>>>>>>>>>>>> 130 snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>>>>>> 131 "thread [%d]
>>>>>>>>>>>>>>>>> frame_buffer[%d].method is different: stack_info_1 =
>>>>>>>>>>>>>>>>> %lx, stack_info_2 = %lx",
>>>>>>>>>>>>>>>>> 132 i, j,
>>>>>>>>>>>>>>>>> si1.frame_buffer[j].method, si2.frame_buffer[j].method);
>>>>>>>>>>>>>>>>> 133 break;
>>>>>>>>>>>>>>>>> 134 } else if (si1.frame_buffer[j].location
>>>>>>>>>>>>>>>>> != si1.frame_buffer[j].location) { /*
>>>>>>>>>>>>>>>>> jvmtiFrameInfo::location */
>>>>>>>>>>>>>>>>> 135 snprintf(err_msg, sizeof(err_msg),
>>>>>>>>>>>>>>>>> 136 "thread [%d]
>>>>>>>>>>>>>>>>> frame_buffer[%d].location is different: stack_info_1 =
>>>>>>>>>>>>>>>>> %ld, stack_info_2 = %ld",
>>>>>>>>>>>>>>>>> 137 i, j,
>>>>>>>>>>>>>>>>> si1.frame_buffer[j].location,
>>>>>>>>>>>>>>>>> si2.frame_buffer[j].location);
>>>>>>>>>>>>>>>>> 138 break;
>>>>>>>>>>>>>>>>> 139 }
>>>>>>>>>>>>>>>>> 140 }
>>>>>>>>>>>>>>>>> 141 }
>>>>>>>>>>>>>>>>> 142 }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Another step would be to create functions that
>>>>>>>>>>>>>>>>> implement a body of each loop.
>>>>>>>>>>>>>>>>> You can use the same techniques to simplify similar
>>>>>>>>>>>>>>>>> place (L127-L138) in the
>>>>>>>>>>>>>>>>> libOneGetThreadListStackTraces.c.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 7/3/20 15:55, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I'm not an Oracle employee, so I cannot know real
>>>>>>>>>>>>>>>>>> request(s) from your customers.
>>>>>>>>>>>>>>>>>> However JDK-8201641 says Dynatrace has requested this
>>>>>>>>>>>>>>>>>> enhancement.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> BTW I haven't heared any request from my customers
>>>>>>>>>>>>>>>>>> about this.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 2020/07/04 4:32, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This difference is not that big to care about.
>>>>>>>>>>>>>>>>>>> I feel this is really rare case and so, does not
>>>>>>>>>>>>>>>>>>> worth these complications.
>>>>>>>>>>>>>>>>>>> Do we have a real request from customers to optimize
>>>>>>>>>>>>>>>>>>> it?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 7/3/20 01:16, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Generally I agree with you, but I have concern
>>>>>>>>>>>>>>>>>>>> about the difference of the result of
>>>>>>>>>>>>>>>>>>>> GetStackTrace() and GetThreadListStackTraces().
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> GetStackTrace: jvmtiFrameInfo
>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces: jvmtiStackInfo
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> jvmtiStackInfo contains thread state, and it is
>>>>>>>>>>>>>>>>>>>> ensured it is the state of the call stack.
>>>>>>>>>>>>>>>>>>>> If we want to get both call stack and thread state,
>>>>>>>>>>>>>>>>>>>> we need to suspend target thread, and call both
>>>>>>>>>>>>>>>>>>>> GetStackTrace() and GetThreadState(). Is it ok?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I was wondering if JDK-8201641 (parent ticket of
>>>>>>>>>>>>>>>>>>>> this change) needed them for profiling (dynatrace?)
>>>>>>>>>>>>>>>>>>>> If it is responsibility of JVMTI agent implementor,
>>>>>>>>>>>>>>>>>>>> I remove this closure.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 2020/07/03 16:45, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> After some thinking I've concluded that I do not
>>>>>>>>>>>>>>>>>>>>> like this optimization
>>>>>>>>>>>>>>>>>>>>> of the GetThreadListStackTraces with
>>>>>>>>>>>>>>>>>>>>> GetSingleStackTraceClosure.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> We may need more opinions on this but these are my
>>>>>>>>>>>>>>>>>>>>> points:
>>>>>>>>>>>>>>>>>>>>> - it adds some complexity and ugliness
>>>>>>>>>>>>>>>>>>>>> - a win is doubtful because it has to be a rare
>>>>>>>>>>>>>>>>>>>>> case, so that total overhead should not be high
>>>>>>>>>>>>>>>>>>>>> - if it is really high for some use cases then
>>>>>>>>>>>>>>>>>>>>> it is up to the user
>>>>>>>>>>>>>>>>>>>>> to optimize it with using GetStackTrace instead
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> In such cases with doubtful overhead I usually
>>>>>>>>>>>>>>>>>>>>> prefer the simplicity.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Good examples where it makes sense to optimize are
>>>>>>>>>>>>>>>>>>>>> checks for target thread to be current thread.
>>>>>>>>>>>>>>>>>>>>> In such cases there is no need to suspend the
>>>>>>>>>>>>>>>>>>>>> target thread, or use a VMop/HandshakeClosure.
>>>>>>>>>>>>>>>>>>>>> For instance, please, see the Monitor functions
>>>>>>>>>>>>>>>>>>>>> with the check: (java_thread == calling_thread).
>>>>>>>>>>>>>>>>>>>>> Getting information for current thread is
>>>>>>>>>>>>>>>>>>>>> frequently used case, e.g. to get info at an event
>>>>>>>>>>>>>>>>>>>>> point.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On 7/2/20 23:29, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi Dan, David,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I uploaded new webrev. Could you review again?
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.04/
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> OneGetThreadListStackTraces.java in this webrev
>>>>>>>>>>>>>>>>>>>>>> would wait until thread state is transited to
>>>>>>>>>>>>>>>>>>>>>> "waiting" with spin wait.
>>>>>>>>>>>>>>>>>>>>>> CountDownLatch::await call as Dan pointed is
>>>>>>>>>>>>>>>>>>>>>> fixed in it :)
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Diff from webrev.03:
>>>>>>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c9aeb7001e50
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On 2020/07/03 14:15, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> On 3/07/2020 2:27 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/03 12:24, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> On 7/2/20 10:50 PM, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry I'm responding here without seeing
>>>>>>>>>>>>>>>>>>>>>>>>>> latest webrev but there is enough context I
>>>>>>>>>>>>>>>>>>>>>>>>>> think ...
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> On 3/07/2020 9:14 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Dan,
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/03 7:16, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/2/20 5:19 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I upload new webrev. Could you review again?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.03/
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L1542: // Get stack trace with handshake
>>>>>>>>>>>>>>>>>>>>>>>>>>>> nit - please add a period at the end.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L1591: *stack_info_ptr = op.stack_info();
>>>>>>>>>>>>>>>>>>>>>>>>>>>> The return parameter should not be
>>>>>>>>>>>>>>>>>>>>>>>>>>>> touched unless the return
>>>>>>>>>>>>>>>>>>>>>>>>>>>> code in 'err' == JVMTI_ERROR_NONE.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> old L1582: if (err == JVMTI_ERROR_NONE) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Please restore this check. The return
>>>>>>>>>>>>>>>>>>>>>>>>>>>> parameter should not
>>>>>>>>>>>>>>>>>>>>>>>>>>>> be touched unless the return code
>>>>>>>>>>>>>>>>>>>>>>>>>>>> in 'err' == JVMTI_ERROR_NONE.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> But op.stack_info() will return NULL if the
>>>>>>>>>>>>>>>>>>>>>>>>>> error is not JVMTI_ERROR_NONE. Are you (Dan)
>>>>>>>>>>>>>>>>>>>>>>>>>> concerned about someone passing in a
>>>>>>>>>>>>>>>>>>>>>>>>>> non-null/initialized out-pointer that will be
>>>>>>>>>>>>>>>>>>>>>>>>>> reset to NULL if there was an error?
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Actually the way we used to test this in POSIX
>>>>>>>>>>>>>>>>>>>>>>>>> tests is to call
>>>>>>>>>>>>>>>>>>>>>>>>> an API with known bad parameters and the
>>>>>>>>>>>>>>>>>>>>>>>>> return parameter ptr
>>>>>>>>>>>>>>>>>>>>>>>>> set to NULL. If the return parameter ptr was
>>>>>>>>>>>>>>>>>>>>>>>>> touched when an
>>>>>>>>>>>>>>>>>>>>>>>>> error should have been detected on an earlier
>>>>>>>>>>>>>>>>>>>>>>>>> parameter, then
>>>>>>>>>>>>>>>>>>>>>>>>> the test failed.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L1272: if (!jt->is_exiting() &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>> (thread_oop != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>> nit - extra parens around the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> second expression.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>> old L1532: _result =
>>>>>>>>>>>>>>>>>>>>>>>>>>>> JVMTI_ERROR_THREAD_NOT_ALIVE;
>>>>>>>>>>>>>>>>>>>>>>>>>>>> This deletion of the _result field
>>>>>>>>>>>>>>>>>>>>>>>>>>>> threw me for a minute and then
>>>>>>>>>>>>>>>>>>>>>>>>>>>> I figured out that the field is
>>>>>>>>>>>>>>>>>>>>>>>>>>>> init to JVMTI_ERROR_THREAD_NOT_ALIVE
>>>>>>>>>>>>>>>>>>>>>>>>>>>> in the constructor.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L1553: if (!jt->is_exiting() &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>> (jt->threadObj() != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>> nit - extra parens around the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> second expression.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>> No comments.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/vmOperations.hpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>> No comments.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/GetThreadListStackTraces.java
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> No comments.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/OneGetThreadListStackTraces.java
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L64: startSignal.countDown();
>>>>>>>>>>>>>>>>>>>>>>>>>>>> I was expecting this to be a call
>>>>>>>>>>>>>>>>>>>>>>>>>>>> to await() instead of
>>>>>>>>>>>>>>>>>>>>>>>>>>>> countDown(). What am I missing here?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> I think this test might be passing
>>>>>>>>>>>>>>>>>>>>>>>>>>>> by accident right now, but...
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Main thread (which call JVMTI functions to
>>>>>>>>>>>>>>>>>>>>>>>>>>> test) should wait until test thread is ready.
>>>>>>>>>>>>>>>>>>>>>>>>>>> So main thread would wait startSignal, and
>>>>>>>>>>>>>>>>>>>>>>>>>>> test thread would count down.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> No!
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> The test thread that previously called
>>>>>>>>>>>>>>>>>>>>>>>>>> obj.wait() now calls latch.await().
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> The main thread that previously called
>>>>>>>>>>>>>>>>>>>>>>>>>> obj.notify() now calls latch.countDown().
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> The main thread continues to spin until it
>>>>>>>>>>>>>>>>>>>>>>>>>> sees the target is WAITING before proceeding
>>>>>>>>>>>>>>>>>>>>>>>>>> with the test.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> If I add spin wait to wait until transit target
>>>>>>>>>>>>>>>>>>>>>>>> thread state is WAITING (as following), we
>>>>>>>>>>>>>>>>>>>>>>>> don't need to call SuspendThread().
>>>>>>>>>>>>>>>>>>>>>>>> Which is better?
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> The original spin-wait loop checking for WAITING
>>>>>>>>>>>>>>>>>>>>>>> is better because it is the only guarantee that
>>>>>>>>>>>>>>>>>>>>>>> the target thread is blocked where you need it
>>>>>>>>>>>>>>>>>>>>>>> to be. suspending the thread is racy as you
>>>>>>>>>>>>>>>>>>>>>>> don't know exactly where the suspend will hit.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>> /* Wait until the thread state transits to
>>>>>>>>>>>>>>>>>>>>>>>> "waiting" */
>>>>>>>>>>>>>>>>>>>>>>>> while (th.getState() != Thread.State.WAITING) {
>>>>>>>>>>>>>>>>>>>>>>>> Thread.onSpinWait();
>>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> For simplify, spin wait is prefer to
>>>>>>>>>>>>>>>>>>>>>>>> OneGetThreadListStackTraces.java in webrev.03.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Here's the flow as I see it:
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>>>>>> - start worker thread
>>>>>>>>>>>>>>>>>>>>>>>>> - startSignal.await()
>>>>>>>>>>>>>>>>>>>>>>>>> - main is now blocked
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> worker thread
>>>>>>>>>>>>>>>>>>>>>>>>> - startSignal.countDown()
>>>>>>>>>>>>>>>>>>>>>>>>> - main is now unblocked
>>>>>>>>>>>>>>>>>>>>>>>>> - stopSignal.await()
>>>>>>>>>>>>>>>>>>>>>>>>> - worker is now blocked
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>>>>>> - checkCallStacks(th)
>>>>>>>>>>>>>>>>>>>>>>>>> - stopSignal.countDown()
>>>>>>>>>>>>>>>>>>>>>>>>> - worker is now unblocked
>>>>>>>>>>>>>>>>>>>>>>>>> - th.join
>>>>>>>>>>>>>>>>>>>>>>>>> - main is now blocked
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> worker thread
>>>>>>>>>>>>>>>>>>>>>>>>> - runs off the end of run()
>>>>>>>>>>>>>>>>>>>>>>>>> - main is now unblocked
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> main thread
>>>>>>>>>>>>>>>>>>>>>>>>> - run off the end of main()
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libGetThreadListStackTraces.c
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L92: jthreads = (jthread
>>>>>>>>>>>>>>>>>>>>>>>>>>>> *)malloc(sizeof(jthread) * num_threads);
>>>>>>>>>>>>>>>>>>>>>>>>>>>> You don't check for malloc() failure.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 'jthreads' is allocated but never freed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> I will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.c
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> L91: result =
>>>>>>>>>>>>>>>>>>>>>>>>>>>> (*jvmti)->SuspendThread(jvmti, thread);
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Why are you suspending the thread?
>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetAllStackTraces() and
>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces() do not require
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the target thread(s)
>>>>>>>>>>>>>>>>>>>>>>>>>>>> to be suspend.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> If you decide not to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> SuspendThread, then you don't need the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> AddCapabilities or the ResumeThread calls.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Test thread might not be entered following
>>>>>>>>>>>>>>>>>>>>>>>>>>> code (stopSignal.await()). We might see
>>>>>>>>>>>>>>>>>>>>>>>>>>> deferent call stack between
>>>>>>>>>>>>>>>>>>>>>>>>>>> GetAllStackTraces() and
>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces(). We cannot
>>>>>>>>>>>>>>>>>>>>>>>>>>> control to freeze call stack of test thread
>>>>>>>>>>>>>>>>>>>>>>>>>>> in Java code.
>>>>>>>>>>>>>>>>>>>>>>>>>>> (I didn't use SuspendThread() at first, but
>>>>>>>>>>>>>>>>>>>>>>>>>>> I saw some errors which causes in above.)
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> So we need to call SuspendThread() to ensure
>>>>>>>>>>>>>>>>>>>>>>>>>>> we can see same call stack.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> If you are checking that the thread is in
>>>>>>>>>>>>>>>>>>>>>>>>>> state WAITING then it cannot escape from that
>>>>>>>>>>>>>>>>>>>>>>>>>> state and you can sample the stack multiple
>>>>>>>>>>>>>>>>>>>>>>>>>> times from any API and get the same result.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> I suspect the errors you saw were from the
>>>>>>>>>>>>>>>>>>>>>>>>>> apparent incorrect use of the CountDownLatch.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> With the flow outlined above, the worker
>>>>>>>>>>>>>>>>>>>>>>>>> thread should be
>>>>>>>>>>>>>>>>>>>>>>>>> nicely blocked in stopSignal.await() when
>>>>>>>>>>>>>>>>>>>>>>>>> stuff is sampled.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/02 15:05, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 11:53 am, Yasumasa Suenaga
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I uploaded new webrev. Could review again?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Updates look fine - thanks.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> One minor nit:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1274 _collector.allocate_and_fill_stacks(1);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1275
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector.set_result(JVMTI_ERROR_NONE);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In the other places where you use
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector you rely on result being
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> initialized to JVMTI_ERROR_NONE, rather
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> than setting it directly after
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allocate_and_fill_stacks().
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 820
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> assert(SafepointSynchronize::is_at_safepoint()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 821
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->is_thread_fully_suspended(false,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &debug_bits) ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 822 current_thread ==
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 823 "at safepoint / handshake or
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> target thread is suspended");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I don't think the suspension check
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is necessary, as even if the target
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is suspended we must still be at a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> safepoint or in a handshake with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it. Makes me wonder if we used to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allow a racy stacktrace operation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on a suspended thread, assuming it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would remain suspended?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This function
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (JvmtiEnvBase::get_stack_trace()) can be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> called to get own stack trace. For
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> example, we can call GetStackTrace() for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> current thread at JVMTI event.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So I changed assert as below:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 820 assert(current_thread ==
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 821
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> SafepointSynchronize::is_at_safepoint() ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 822 current_thread ==
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 823 "call by myself / at safepoint /
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> at handshake");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yep good catch. I hope current tests
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> caught that.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> They would be tested in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti/GetStackTrace/getstacktr001/
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (own call stacks), and getstacktr003 (call
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> stacks in other thread).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Speaking of tests ...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In the native code I think you need to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> check the success of all JNI methods that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> can throw exceptions - otherwise I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> believe the tests may trigger warnings if
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -Xcheck:jni is used with them. See for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> example:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I updated testcases to check JNI and JVMTI
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> function calls.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In the Java code the target thread:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 45 public void run() {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 46 try {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 47 synchronized (lock) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 48 lock.wait();
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 49 System.out.println("OK");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 50 }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is potentially susceptible to spurious
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wakeups. Using a CountDownLatch would be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> robust.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/01 8:48, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 9:05 am, Yasumasa Suenaga
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIUC at this point the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _calling_thread is the current
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread, so we can use:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ResourceMark rm(_calling_thread);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If so, we can call make_local() in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> L1272 without JavaThread (or we can
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pass current thread to make_local()).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Is it right?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1272
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_oop),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1273 jt, thread_oop);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry I got confused, _calling_thread
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> may not be the current thread as we
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> could be executing the handshake in the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> target thread itself. So the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ResourceMark is correct as-is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (implicitly for current thread).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The argument to fill_frames will be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> used in the jvmtiStackInfo and passed
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> back to the _calling_thread, so it must
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be created via
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> make_local(_calling_thread, ...) as you
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> presently have.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/07/01 7:05, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 1/07/2020 12:17 am, Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thank you for reviewing! I will
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> update new webrev tomorrow.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 466 class
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public StackObj {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 498 class VM_GetAllStackTraces :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 499 private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 500 JavaThread *_calling_thread;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 501 jint _final_thread_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 502 MultipleStackTracesCollector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can't have a StackObj as a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> member of another class like that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> as it may not be on the stack. I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> think MultipleStackTracesCollector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should not extend any allocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class, and should always be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> embedded directly in another class.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm not sure what does mean "embedded".
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Is it ok as below?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class MultipleStackTracesCollector {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class GetAllStackTraces : public
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yes that I what I meant.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/06/30 22:22, David Holmes
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 30/06/2020 10:05 am, Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi David, Serguei,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I updated webrev for 8242428.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Could you review again?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change migrate to use direct
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> handshake for GetStackTrace() and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces() (when
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_count == 1).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This looks really good now! I only
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have a few nits below. There is one
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thing I don't like about it but it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> requires a change to the main
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Handshake logic to address - in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> JvmtiEnv::GetThreadListStackTraces
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you have to create a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ThreadsListHandle to convert the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jthread to a JavaThread, but then
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the Handshake::execute_direct
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> creates another ThreadsListHandle
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> internally. That's a waste. I will
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> discuss with Robbin and file a RFE
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to have an overload of
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> execute_direct that takes an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> existing TLH. Actually it's worse
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> than that because we have another
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> TLH in use at the entry point for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the JVMTI functions, so I think
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> there may be some scope for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> simplifying the use of TLH
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instances - future RFE.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.hpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 451 GetStackTraceClosure(JvmtiEnv
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> *env, jint start_depth, jint
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> max_count,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 452 jvmtiFrameInfo* frame_buffer,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jint* count_ptr)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 453 :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> HandshakeClosure("GetStackTrace"),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 454 _env(env),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _start_depth(start_depth),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _max_count(max_count),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 455 _frame_buffer(frame_buffer),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _count_ptr(count_ptr),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 456
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _result(JVMTI_ERROR_THREAD_NOT_ALIVE)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Nit: can you do one initializer per
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> line please.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This looks wrong:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 466 class
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public StackObj {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 498 class VM_GetAllStackTraces :
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> public VM_Operation {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 499 private:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 500 JavaThread *_calling_thread;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 501 jint _final_thread_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 502 MultipleStackTracesCollector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can't have a StackObj as a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> member of another class like that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> as it may not be on the stack. I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> think MultipleStackTracesCollector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should not extend any allocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class, and should always be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> embedded directly in another class.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 481
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MultipleStackTracesCollector(JvmtiEnv
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> *env, jint max_frame_count) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 482 _env = env;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 483 _max_frame_count =
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> max_frame_count;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 484 _frame_count_total = 0;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 485 _head = NULL;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 486 _stack_info = NULL;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 487 _result = JVMTI_ERROR_NONE;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 488 }
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As you are touching this can you
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> change it to use an initializer
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> list as you did for the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> HandshakeClosure, and please keep
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> one item per line.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 820
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> assert(SafepointSynchronize::is_at_safepoint()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 821
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->is_thread_fully_suspended(false,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &debug_bits) ||
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 822 current_thread ==
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> java_thread->active_handshaker(),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 823 "at safepoint / handshake or
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> target thread is suspended");
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I don't think the suspension check
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is necessary, as even if the target
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is suspended we must still be at a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> safepoint or in a handshake with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it. Makes me wonder if we used to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allow a racy stacktrace operation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on a suspended thread, assuming it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would remain suspended?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1268 oop thread_oop =
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jt->threadObj();
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1269
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1270 if (!jt->is_exiting() &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (jt->threadObj() != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You can use thread_oop in line 1270.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1272
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread_oop),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1273 jt, thread_oop);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It is frustrating that this entire
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> call chain started with a jthread
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> reference, which we converted to a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> JavaThread, only to eventually need
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to convert it back to a jthread! I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> think there is some scope for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> simplification here but not as part
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of this change.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1271 ResourceMark rm;
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIUC at this point the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> _calling_thread is the current
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread, so we can use:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ResourceMark rm(_calling_thread);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Please add @bug lines to the tests.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm still pondering the test logic
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> but wanted to send this now.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetThreadListStackTrace (for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces) and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetAllStackTraces (for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetAllStackTraces) have inherited
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetMultipleStackTraces VM
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> operation which provides the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> feature to generate
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> jvmtiStackInfo. I modified
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VM_GetMultipleStackTraces to a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> normal C++ class to share with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> HandshakeClosure for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTraces
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetSingleStackTraceClosure).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Also I added new testcases which
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> test GetThreadListStackTraces()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> with thread_count == 1 and with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all threads.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change has been tested in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jvmti
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jdwp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jdi
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jdwp.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 2020/06/24 15:50, Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Please review this change:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> JBS:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8242428
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change replace following VM
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> operations to direct handshake.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - VM_GetFrameCount
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetFrameCount())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - VM_GetFrameLocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetFrameLocation())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - VM_GetThreadListStackTraces
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (GetThreadListStackTrace())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - VM_GetCurrentLocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GetThreadListStackTrace() uses
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> direct handshake if thread count
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> == 1. In other case (thread count
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > 1), it would be performed as VM
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> operation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (VM_GetThreadListStackTraces).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Caller of VM_GetCurrentLocation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (JvmtiEnvThreadState::reset_current_location())
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> might be called at safepoint. So
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I added safepoint check in its
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> caller.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This change has been tested in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jvmti
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serviceability/jdwp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jvmti
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vmTestbase/nsk/jdi vmTestbase/ns
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> k/jdwp.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Also I tested it on submit repo,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> then it has execution error
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> due to dependency error. So I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> think it does not occur by this
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> change.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>
>>>
>>
More information about the serviceability-dev
mailing list