RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jul 10 22:00:52 UTC 2020
On 7/10/20 3:29 AM, Yasumasa Suenaga wrote:
> Thanks Patricio!
>
> I uploaded new webrev. Could you review again?
> It passed all tests on submit repo, and passed jtreg tests about JVMTI.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.10/
> Diff from webrev.09:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.10/incremental/
test/hotspot/jtreg/serviceability/jvmti/GetThreadListStackTraces/libOneGetThreadListStackTraces.cpp
No comments.
Thumbs up on the incremental.
Dan
>
>
> Yasumasa
>
>
> On 2020/07/10 14:49, Patricio Chilano wrote:
>> 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