RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
David Holmes
david.holmes at oracle.com
Sat Jul 11 13:50:12 UTC 2020
On 11/07/2020 11:52 am, Yasumasa Suenaga wrote:
> Thanks Dan!
>
> David, Serguei, are you ok to this change?
Yes it seems fine.
In relation to an earlier comment:
> I replaced %p to %lx, and also cast values to unsigned long
Never use long or unsigned long as they can be different sizes on
different platforms (specifically long is not 64-bit on Windows).
Thanks,
David
-----
>
> Yasumasa
>
>
> On 2020/07/11 7:00, Daniel D. Daugherty wrote:
>> 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