[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
From: "Reingruber, Richard" <richard.reingruber () sap ! com>
Date: 2020-08-25 7:34:24
Message-ID: AM0PR0202MB3331E4BFDEE28E6D68ABAA2A9B570 () AM0PR0202MB3331 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
[Attachment #2 (text/plain)]
Hi Serguei,
thanks for your review.
> These two lines can be removed:
> 391 jvmtiEnv* jvmti;
> 406 ::jvmti = jvmti;
> No need in another webrev if you fix it.
I've made this change and fixed the copyright year in
jvmtiImpl.hpp. Unfortunately I got no results from the submit repo [1].
I've pushed again to submit. Will push to the jdk master repo when this job
reports success.
Thanks,
Richard.
[1] https://mail.openjdk.java.net/pipermail/jdk-submit-changes/2020-August/012091.html
From: serguei.spitsyn@oracle.com <serguei.spitsyn@oracle.com>
Sent: Freitag, 21. August 2020 18:48
To: Reingruber, Richard <richard.reingruber@sap.com>; David Holmes \
<david.holmes@oracle.com>; serviceability-dev@openjdk.java.net
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi Richard,
It looks good, thank you for the update!
These two lines can be removed:
391 jvmtiEnv* jvmti;
406 ::jvmti = jvmti;
No need in another webrev if you fix it.
Thanks,
Serguei
On 8/21/20 09:09, Reingruber, Richard wrote:
Hi Serguei,
I have prepared a new webrev based on your suggestions.
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6.inc/
Thanks,
Richard.
______
From: serguei.spitsyn@oracle.com<mailto:serguei.spitsyn@oracle.com> \
<serguei.spitsyn@oracle.com><mailto:serguei.spitsyn@oracle.com>
Sent: Freitag, 21. August 2020 11:22
To: Reingruber, Richard \
<richard.reingruber@sap.com><mailto:richard.reingruber@sap.com>; David Holmes \
<david.holmes@oracle.com><mailto:david.holmes@oracle.com>; \
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi Richard,
Thank you for the update, it looks really nice.
Just several more minor comments though (I hope, the last ones).
Should I rename the variable to spinWaitCycles or something similar?
Yes, waitCycles would be better and more consistent.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.udiff.html
81 * The wait time is given in cycles.
82 */ 83 public int waitTime;
...
93 waitTime = 1;
This line 82 can be removed if you rename waitTime to waitCycles.
It is better to initialize waitCycles at definition and remove the line 93.
146 public static void msg(String m) {
147 System.out.println("### Java-Test: " + m);
148 }
One of the de-facto standard names for such methods is "log".
80 * Wait time in native, i.e. with walkable stack, after notifying agent \
thread to do GetLocalObject() call.
89 msg("Test how many frames fit on the stack by performing recursive calls \
until StackOverflowError is thrown");
Could you, please, reballance the two long lines above?
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html
There are several spots that can be simplified a little bit:
95 jvmtiError result;
96
97 result = jvmti->GetErrorName(errCode, &errMsg);
==>
jvmtiError result = jvmti->GetErrorName(errCode, &errMsg);
The same is true for for the following cases:
115 err = jvmti->RawMonitorEnter(glws_monitor);
125 err = jvmti->RawMonitorExit(glws_monitor);
135 err = jvmti->RawMonitorWait(glws_monitor, 0);
145 err = jvmti->RawMonitorNotify(glws_monitor);
155 err = jvmti->DestroyRawMonitor(glws_monitor);
173 if (errMsg != NULL) {
An extra space before NULL.
89 static jvmtiEnv* jvmti_global = NULL;
276 jvmtiEnv* jvmti = jvmti_global;
308 jvmtiEnv* jvmti = jvmti_global;
330 jvmtiEnv* jvmti = jvmti_global;
...
409 jvmtiEnv* jvmti;
419 res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_9);
424 jvmti_global = jvmti;
Normal practice is to name the "global_jvmti" as "jvmti".
Then there is no need to set it at the start of each function.
Thanks,
Serguei
On 8/20/20 23:47, Reingruber, Richard wrote:
Hi Serguei,
Sorry for the delay in reply and thank you for the update.
I like it in general. There are some minor comments though.
Excellent, thanks :)
I've prepared webrev.5.
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html
112 waitTime = (waitTime << 1); // double wait time
113 if (waitTime >= M || waitTime < 0) {
114 waitTime = 1; // reset when too long
115 }
The M is too big for time.
"waitTime" is roughly the number of cycles spent in a spin wait. M ~= 10^6
cycles does not seem too long. Should I rename the variable to spinWaitCycles or
something similar?
What about something like this:
waitTime = (waitTime << 1) % 32;
or
waitTime = (waitTime << 1) & 32;
I went for
// Double wait time, but limit to roughly 10^6 cycles.
waitTime = (waitTime << 1) & (M - 1);
waitTime = waitTime == 0 ? 1 : waitTime;
Masking the waitTime with % 32 is too small. In my experiments with fastdebug
builds I got the crash often with a waitTime of 8K on a Linux server and 256K on
my Windows notebook.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udiff.html
// - Wait for the target thread to either start a new test iteration or to
+// signal shutdown.
A suggestion to replace: "to either start" => "either to start".
Ok, done.
+// Shutdown is signalled by setting test_state to ShutDown. The agent reacts
+// to it by changing test_state to Terminated and then it exits.
The second "it" is not needed: "then it exits" => "then exits".
Ok, done.
+// ... It sets the shared variable test_state
+// to TargetInNative and then it uses the glws_monitor to send the
The second "it" is not needed.
Ok, done.
+ monitor_enter(jvmti, env, glws_monitor, AT_LINE);
+ monitor_notify(jvmti, env, glws_monitor, AT_LINE);
+ monitor_wait(jvmti, env, glws_monitor, AT_LINE);
+ monitor_exit(jvmti, env, glws_monitor, AT_LINE);
+ monitor_destroy(jvmti, env, glws_monitor, AT_LINE);
There is only one lock.
It'd be more simple to directly use it in the called functions and get rid of the \
parameter.
Just a suggestion, it is up to you to decide.
Ok, done.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html
I'd suggest to refactor the lines 213 and 239-257 to a separate function \
test_GetLocalObject(jvmti, depth).
240 jobject local_val;
Better to rename it to local_obj or just obj.
Ok, done.
There are still problems with the indent.
I reformatted the file using 2 space indentation like in other C++ sources. I didn't \
include the
indentation change in the delta webrev.
Thanks,
Richard.
______________________
From: mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com
Sent: Donnerstag, 20. August 2020 04:42
To: Reingruber, Richard mailto:richard.reingruber@sap.com; David Holmes \
mailto:david.holmes@oracle.com; mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi Richard,
Sorry for the delay in reply and thank you for the update.
I like it in general. There are some minor comments though.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html
112 waitTime = (waitTime << 1); // double wait time
113 if (waitTime >= M || waitTime < 0) {
114 waitTime = 1; // reset when too long
115 }
The M is too big for time.
What about something like this:
waitTime = (waitTime << 1) % 32;
or
waitTime = (waitTime << 1) & 32;
You can choose a better number instead of 32.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udiff.html
// - Wait for the target thread to either start a new test iteration or to
+// signal shutdown.
A suggestion to replace: "to either start" => "either to start".
+// Shutdown is signalled by setting test_state to ShutDown. The agent reacts
+// to it by changing test_state to Terminated and then it exits.
The second "it" is not needed: "then it exits" => "then exits".
+// ... It sets the shared variable test_state
+// to TargetInNative and then it uses the glws_monitor to send the
The second "it" is not needed.
+ monitor_enter(jvmti, env, glws_monitor, AT_LINE);
+ monitor_notify(jvmti, env, glws_monitor, AT_LINE);
+ monitor_wait(jvmti, env, glws_monitor, AT_LINE);
+ monitor_exit(jvmti, env, glws_monitor, AT_LINE);
+ monitor_destroy(jvmti, env, glws_monitor, AT_LINE);
There is only one lock.
It'd be more simple to directly use it in the called functions and get rid of the \
parameter.
Just a suggestion, it is up to you to decide.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html
I'd suggest to refactor the lines 213 and 239-257 to a separate function \
test_GetLocalObject(jvmti, depth).
240 jobject local_val;
Better to rename it to local_obj or just obj.
There are still problems with the indent.
The indent 4 is mostly used.
However there are still fragments with the indent 2:
112 static void monitor_enter(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, const \
char* loc) {
113 jvmtiError err;
114
115 err = jvmti->RawMonitorEnter(mon);
116 if (err != JVMTI_ERROR_NONE) {
117 ShowErrorMessage(jvmti, err, loc);
118 env->FatalError(loc);
119 }
120 }
121
122 static void monitor_exit(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, const \
char* loc) {
123 jvmtiError err;
124
125 err = jvmti->RawMonitorExit(mon);
126 if (err != JVMTI_ERROR_NONE) {
127 ShowErrorMessage(jvmti, err, loc);
128 env->FatalError(loc);
129 }
130 }
131
132 static void monitor_wait(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, const \
char* loc) {
133 jvmtiError err;
134
135 err = jvmti->RawMonitorWait(mon, 0);
136 if (err != JVMTI_ERROR_NONE) {
137 ShowErrorMessage(jvmti, err, loc);
138 env->FatalError(loc);
139 }
140 }
141
142 static void monitor_notify(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, \
const char* loc) {
143 jvmtiError err;
144
145 err = jvmti->RawMonitorNotify(mon);
146 if (err != JVMTI_ERROR_NONE) {
147 ShowErrorMessage(jvmti, err, loc);
148 env->FatalError(loc);
149 }
150 }
151
152 static void monitor_destroy(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, \
const char* loc) {
153 jvmtiError err;
154
155 err = jvmti->DestroyRawMonitor(mon);
156 if (err != JVMTI_ERROR_NONE) {
157 ShowErrorMessage(jvmti, err, loc);
158 env->FatalError(loc);
159 }
...
160 }
196 while (target_thread == NULL) {
197 monitor_wait(jvmti, env, glws_monitor, AT_LINE);
198 }
...
220 while (test_state != TargetInNative) {
221 if (test_state == ShutDown) {
222 test_state = Terminated;
223 monitor_notify(jvmti, env, glws_monitor, AT_LINE);
224 monitor_exit(jvmti, env, glws_monitor, AT_LINE);
225 return;
226 }
227 monitor_wait(jvmti, env, glws_monitor, AT_LINE);
228 }
...
263 // Called by target thread after building a large stack.
264 // By calling this native method, the thread's stack becomes walkable.
265 // It notifies the agent to do the GetLocalObject() call and then races
266 // it to make its stack not walkable by returning from the native call.
267 JNIEXPORT void JNICALL
268 Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocal(JNIEnv *env, jclass cls, \
jint depth, jlong waitCycles) {
269 jvmtiEnv* jvmti = jvmti_global;
270
271 monitor_enter(jvmti, env, glws_monitor, AT_LINE);
272
273 // Set depth_for_get_local and notify agent that the target thread is ready \
for the GetLocalObject() call
274 depth_for_get_local = depth;
275 test_state = TargetInNative;
276
277 monitor_notify(jvmti, env, glws_monitor, AT_LINE);
278
279 // Wait for agent thread to read depth_for_get_local and do the \
GetLocalObject() call
280 while (test_state != AgentInGetLocal) {
281 monitor_wait(jvmti, env, glws_monitor, AT_LINE);
282 }
283
284 // Reset state to Initial
285 test_state = Initial;
286
287 monitor_exit(jvmti, env, glws_monitor, AT_LINE);
288
289 // Wait a little until agent thread is in unsafe stack walk.
290 // This needs to be a spin wait or sleep because we cannot get a notification
291 // from there.
292 while (--waitCycles > 0) {
293 dummy_counter++;
294 }
295 }
...
299 JNIEXPORT void JNICALL
300 Java_GetLocalWithoutSuspendTest_shutDown(JNIEnv *env, jclass cls) {
301 jvmtiEnv* jvmti = jvmti_global;
302
303 monitor_enter(jvmti, env, glws_monitor, AT_LINE);
304
305 // Notify agent thread to shut down
306 test_state = ShutDown;
307 monitor_notify(jvmti, env, glws_monitor, AT_LINE);
308
309 // Wait for agent to terminate
310 while (test_state != Terminated) {
311 monitor_wait(jvmti, env, glws_monitor, AT_LINE);
312 }
313
314 monitor_exit(jvmti, env, glws_monitor, AT_LINE);
315
316 // Destroy glws_monitor
317 monitor_destroy(jvmti, env, glws_monitor, AT_LINE);
318 }
Thanks,
Serguei
On 8/14/20 07:06, Reingruber, Richard wrote:
Hi Serguei,
thanks for the feedback. I have implemented your suggestions and created a new
webrev:
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/
Please find my replies to your comments below.
Best regards,
Richard.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html
33 * the stack walk. The target thread's stack is walkable while in \
native. After sending the notification it
...
54 * @param depth Depth of target frame for GetLocalObject() call. Should be \
large value to prolong the unsafe stack walk.
55 * @param waitTimeInNativeAfterNotify Time to wait after notify with \
walkable stack before returning an becoming unsafe again.
...
71 * Wait time in native, i.e. with walkable stack, after notifying agent \
thread to do GetLocalObject() call.
...
89 msg((now -start) + " ms Iteration : " + iterations + " \
waitTimeInNativeAfterNotify : " + waitTimeInNativeAfterNotify);
Could you, please, re-balance the lines above to make them shorter?
Ok, done.
90 int newTargetDepth = recursiveMethod(0, targetDepth);
91 if (newTargetDepth < targetDepth) {
92 msg("StackOverflowError during test.");
93 msg("Old target depth: " + targetDepth);
94 msg("Retry with new target depth: " + newTargetDepth);
95 targetDepth = newTargetDepth;
96 }
A comment is needed to explain why a StackOverflowError is not desired.
At least, it is not obvious initially.
73 public int waitTimeInNativeAfterNotify;
This name is unreasonably long which makes the code less readable.
I'd suggest to reduce it to waitTime.
Ok, done.
103 notifyAgentToGetLocalAndWaitShortly(-1, 1);
...
119 notifyAgentToGetLocalAndWaitShortly(depth - 100, \
waitTimeInNativeAfterNotify);
It is better to provide a short comment before each call explaining what it is doing.
For instance, it is not clear why the call at the line 103 is needed.
Why do we need to notify the agent to GetLocal for the second time?
The test is repeated TEST_ITERATIONS times. In each iteration the agent calls
GetLocal racing the target thread returning from the native call. The last call
in line 103 ist the shutdown signal.
Can it be refactored into a separate native method?
I've made the shutdown process more explicit with the new native method
shutDown() which sets thest_state to ShutDown.
Then the the function name can be reduced to 'notifyAgentToGetLocal'.
This long name does not give enough context anyway.
Ok, done.
85 long iterations = 0;
87 do {
...
97 iterations++;
...
102 } while (iterations < TEST_ITERATIONS);
Why a more explicit 'for' or 'while' loop is not used here? :
for (long iter = 0; iter < TEST_ITERATIONS; iter++) {
I have converted the loop into a for loop.
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html
The indent in this file varies. It is better to keep it the same: 4 or 2.
Yes, I noticed this. I have not corrected it yet, because I didn't want to
pullute the incremental webrev with that change. Would you like me to fix the
indentation now to 2 spaces or do it as a last step?
60 AgentCallingGetLocalObject // The target thread waits for the agent to call
I'd suggest to rename the constant to 'AgentInGetLocal'.
Ok, done.
150 GetLocalWithoutSuspendTestThreadLoop(jvmtiEnv * jvmti, JNIEnv* env, void * arg) \
{
It is better rename the function to TestThreadLoop.
Would AgentThreadLoop be ok too?
You can add a comment before to explain some basic about what it is doing.
Ok, done.
167 printf("*** AGENT: GetLocalWithoutSuspendTestThreadLoop thread started. \
Polling thread '%s' for local variables\n",
It is better to get rid of leading stars in all messages.
Ok, done.
176 // the native method \
Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocalAndWaitShortly
The part 'Java_GetLocalWithoutSuspendTest_' can be removed from the function name.
Ok, done.
---
From: mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com
Sent: Freitag, 14. August 2020 10:11
To: Reingruber, Richard mailto:richard.reingruber@sap.com; David Holmes \
mailto:david.holmes@oracle.com; mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi Richard,
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html
33 * the stack walk. The target thread's stack is walkable while in \
native. After sending the notification it
...
54 * @param depth Depth of target frame for GetLocalObject() call. Should be \
large value to prolong the unsafe stack walk.
55 * @param waitTimeInNativeAfterNotify Time to wait after notify with \
walkable stack before returning an becoming unsafe again.
...
71 * Wait time in native, i.e. with walkable stack, after notifying agent \
thread to do GetLocalObject() call.
...
89 msg((now -start) + " ms Iteration : " + iterations + " \
waitTimeInNativeAfterNotify : " + waitTimeInNativeAfterNotify);
Could you, please, re-balance the lines above to make them shorter?
90 int newTargetDepth = recursiveMethod(0, targetDepth);
91 if (newTargetDepth < targetDepth) {
92 msg("StackOverflowError during test.");
93 msg("Old target depth: " + targetDepth);
94 msg("Retry with new target depth: " + newTargetDepth);
95 targetDepth = newTargetDepth;
96 }
A comment is needed to explain why a StackOverflowError is not desired.
At least, it is not obvious initially.
73 public int waitTimeInNativeAfterNotify;
This name is unreasonably long which makes the code less readable.
I'd suggest to reduce it to waitTime.
103 notifyAgentToGetLocalAndWaitShortly(-1, 1);
...
119 notifyAgentToGetLocalAndWaitShortly(depth - 100, \
waitTimeInNativeAfterNotify);
It is better to provide a short comment before each call explaining what it is doing.
For instance, it is not clear why the call at the line 103 is needed.
Why do we need to notify the agent to GetLocal for the second time?
Can it be refactored into a separate native method?
Then the the function name can be reduced to 'notifyAgentToGetLocal'.
This long name does not give enough context anyway.
85 long iterations = 0;
87 do {
...
97 iterations++;
...
102 } while (iterations < TEST_ITERATIONS);
Why a more explicit 'for' or 'while' loop is not used here? :
for (long iter = 0; iter < TEST_ITERATIONS; iter++) {
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html
The indent in this file varies. It is better to keep it the same: 4 or 2.
60 AgentCallingGetLocalObject // The target thread waits for the agent to call
I'd suggest to rename the constant to 'AgentInGetLocal'.
150 GetLocalWithoutSuspendTestThreadLoop(jvmtiEnv * jvmti, JNIEnv* env, void * arg) \
{
It is better rename the function to TestThreadLoop.
You can add a comment before to explain some basic about what it is doing.
167 printf("*** AGENT: GetLocalWithoutSuspendTestThreadLoop thread started. \
Polling thread '%s' for local variables\n",
It is better to get rid of leading stars in all messages.
176 // the native method \
Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocalAndWaitShortly
The part 'Java_GetLocalWithoutSuspendTest_' can be removed from the function name.
I'm still reviewing the test native agent code.
Thanks,
Serguei
On 8/11/20 03:02, Reingruber, Richard wrote:
Hi David and Serguei,
On 11/08/2020 3:21 am, mailto:serguei.spitsyn@oracle.com wrote:
Hi Richard and David,
The implementation looks good to me.
But I do not understand what the test is doing with all this counters
and recursions.
For instance, these fragments:
86 recursions = 0;
87 try {
88 recursiveMethod(1<<20);
89 } catch (StackOverflowError e) {
90 msg("Caught StackOverflowError as expected");
91 }
92 int target_depth = recursions-100; // spaces are missed around the \
'-' sigh
It is not obvious that the 'recursion' is updated in the recursiveMethod.
I would suggestto make it more explicit:
recursiveMethod(M);
int target_depth = M - 100;
Then the variable 'recursions' can be removed or become local.
The recursiveMethod takes in the maximum recursions to try and updates
the recursions variable to record how many recursions were possible - so:
target_depth = <actual recursions> - 100;
Possibly recursiveMethod could return the actual recursions instead of
using the global variables?
I've eliminated the static 'recursions' variable. recursiveMethod() now returns
the depth at which the recursion was ended. I hesitated doing this, because I
had to handle the StackOverflowError with all those frames still on stack. But
the handler is empty, so it should not cause problems.
This is the new webrev (as posted previously):
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/
Thanks, Richard.
-----Original Message-----
From: David Holmes mailto:david.holmes@oracle.com
Sent: Dienstag, 11. August 2020 04:00
To: mailto:serguei.spitsyn@oracle.com; Reingruber, Richard \
mailto:richard.reingruber@sap.com; mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi Serguei,
On 11/08/2020 3:21 am, mailto:serguei.spitsyn@oracle.com wrote:
Hi Richard and David,
The implementation looks good to me.
But I do not understand what the test is doing with all this counters
and recursions.
For instance, these fragments:
86 recursions = 0;
87 try {
88 recursiveMethod(1<<20);
89 } catch (StackOverflowError e) {
90 msg("Caught StackOverflowError as expected");
91 }
92 int target_depth = recursions-100; // spaces are missed around the \
'-' sigh
It is not obvious that the 'recursion' is updated in the recursiveMethod.
I would suggestto make it more explicit:
recursiveMethod(M);
int target_depth = M - 100;
Then the variable 'recursions' can be removed or become local.
The recursiveMethod takes in the maximum recursions to try and updates
the recursions variable to record how many recursions were possible - so:
target_depth = <actual recursions> - 100;
Possibly recursiveMethod could return the actual recursions instead of
using the global variables?
David
-----
This method will be:
47 private static final int M = 1 << 20;
...
121 public long recursiveMethod(int depth) {
123 if (depth == 0) {
124 notifyAgentToGetLocalAndWaitShortly(M - 100, \
waitTimeInNativeAfterNotify);
126 } else {
127 recursiveMethod(--depth);
128 }
129 }
At least, he test is missing the comments explaining all these.
Thanks,
Serguei
On 8/9/20 22:35, David Holmes wrote:
Hi Richard,
On 31/07/2020 5:28 pm, Reingruber, Richard wrote:
Hi,
I rebase the fix after JDK-8250042.
New webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.2/
The general fix for this seems good. A minor nit:
588 if (!is_assignable(signature, ob_k, Thread::current())) {
You know that the current thread is the VMThread so can use
VMThread::vm_thread().
Similarly for this existing code:
694 Thread* current_thread = Thread::current();
---
Looking at the test code ... I'm less clear on exactly what is
happening and the use of spin-waits raises some red-flags for me in
terms of test reliability on different platforms. The "while
(--waitCycles > 0)" loop in particular offers no certainty that the
agent thread is executing anything in particular. And the use of the
spin_count as a guide to future waiting time seems somewhat arbitrary.
In all seriousness I got a headache trying to work out how the test
was expecting to operate. Some parts could be simplified using raw
monitors, I think. But there's no sure way to know the agent thread is
in the midst of the stackwalk when the target thread wants to leave
the native code. So I understand what you are trying to achieve here,
I'm just not sure how reliably it will actually achieve it.
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp
32 static volatile jlong spinn_count = 0;
Using a 64-bit counter seems like it will be a problem on 32-bit systems.
Should be spin_count not spinn_count.
36 // Agent thread waits for value != 0, then performas the JVMTI
call to get local variable.
typo: performas
Thanks,
David
-----
Thanks, Richard.
-----Original Message-----
From: serviceability-dev mailto:serviceability-dev-retn@openjdk.java.net
On Behalf Of Reingruber, Richard
Sent: Montag, 27. Juli 2020 09:45
To: mailto:serguei.spitsyn@oracle.com; mailto:serviceability-dev@openjdk.java.net
Subject: [CAUTION] RE: RFR(S) 8249293: Unsafe stackwalk in
VM_GetOrSetLocal::doit_prologue()
Hi Serguei,
> I tested it on Linux and Windows but not yet on MacOS.
The test succeeded now on all platforms.
Thanks, Richard.
-----Original Message-----
From: Reingruber, Richard
Sent: Freitag, 24. Juli 2020 15:04
To: mailto:serguei.spitsyn@oracle.com; mailto:serviceability-dev@openjdk.java.net
Subject: RE: RFR(S) 8249293: Unsafe stackwalk in
VM_GetOrSetLocal::doit_prologue()
Hi Serguei,
The fix itself looks good to me.
thanks for looking at the fix.
I still need another look at new test.
Could you, please, convert the agent of new test to C++?
It will make it a little bit simpler.
Sure, here is the new webrev.1 with a C++ version of the test agent:
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.1/
I tested it on Linux and Windows but not yet on MacOS.
Thanks,
Richard.
-----Original Message-----
From: mailto:serguei.spitsyn@oracle.com mailto:serguei.spitsyn@oracle.com
Sent: Freitag, 24. Juli 2020 00:00
To: Reingruber, Richard mailto:richard.reingruber@sap.com;
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR(S) 8249293: Unsafe stackwalk in
VM_GetOrSetLocal::doit_prologue()
Hi Richard,
Thank you for filing the CR and taking care about it!
The fix itself looks good to me.
I still need another look at new test.
Could you, please, convert the agent of new test to C++?
It will make it a little bit simpler.
Thanks,
Serguei
On 7/20/20 01:15, Reingruber, Richard wrote:
Hi,
please help review this fix for VM_GetOrSetLocal. It moves the
unsafe stackwalk from the vm
operation prologue before the safepoint into the doit() method
executed at the safepoint.
Webrev:
http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.0/index.html
Bug: https://bugs.openjdk.java.net/browse/JDK-8249293
According to the JVMTI spec on local variable access it is not
required to suspend the target thread
T [1]. The operation will simply fail with
JVMTI_ERROR_NO_MORE_FRAMES if T is executing
bytecodes. It will succeed though if T is blocked because of
synchronization or executing some native
code.
The issue is that in the latter case the stack walk in
VM_GetOrSetLocal::doit_prologue() to prepare
the access to the local variable is unsafe, because it is done
before the safepoint and it races
with T returning to execute bytecodes making its stack not walkable.
The included test shows that
this can crash the VM if T wins the race.
Manual testing:
- new test
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java
- test/hotspot/jtreg/vmTestbase/nsk/jvmti
- test/hotspot/jtreg/serviceability/jvmti
Nightly regression tests @SAP: JCK and JTREG, also in Xcomp mode,
SPECjvm2008, SPECjbb2015,
Renaissance Suite, SAP specific tests with fastdebug and release
builds on all platforms
Thanks, Richard.
[1]
https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#local
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"> <head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.changed
{mso-style-name:changed;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 2.0cm 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Hi Serguei,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">thanks for your review.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> These two lines can be removed:<o:p></o:p></p>
<p class="MsoNormal">> 391 jvmtiEnv* jvmti;<o:p></o:p></p>
<p class="MsoNormal">> 406 ::jvmti = jvmti;<o:p></o:p></p>
<p class="MsoNormal">> No need in another webrev if you fix it.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I've made this change and fixed the copyright year \
in<o:p></o:p></p> <p class="MsoNormal">jvmtiImpl.hpp. Unfortunately I got no results \
from the submit repo [1].<o:p></o:p></p> <p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I've pushed again to submit. Will push to the jdk master repo \
when this job<o:p></o:p></p> <p class="MsoNormal">reports success.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
<p class="MsoNormal">Richard.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">[1] \
https://mail.openjdk.java.net/pipermail/jdk-submit-changes/2020-August/012091.html<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b>From:</b> serguei.spitsyn@oracle.com \
<serguei.spitsyn@oracle.com> <br>
<b>Sent:</b> Freitag, 21. August 2020 18:48<br>
<b>To:</b> Reingruber, Richard <richard.reingruber@sap.com>; David Holmes \
<david.holmes@oracle.com>; serviceability-dev@openjdk.java.net<br> \
<b>Subject:</b> Re: RFR(S) 8249293: Unsafe stackwalk in \
VM_GetOrSetLocal::doit_prologue()<o:p></o:p></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Hi Richard,<br>
<br>
It looks good, thank you for the update!<br>
<br>
These two lines can be removed:<o:p></o:p></p>
<pre> 391 jvmtiEnv* jvmti;<o:p></o:p></pre>
<pre><span class="changed"> 406 ::jvmti = jvmti;</span><o:p></o:p></pre>
<p class="MsoNormal">No need in another webrev if you fix it.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 8/21/20 09:09, Reingruber, Richard wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I have prepared a new webrev based on your suggestions.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Webrev: <a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6/</a><o:p></o:p></pre>
<pre>Delta: <a \
href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6.inc/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.6.inc/</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>______<o:p></o:p></pre>
<pre>From: <a href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> \
<a href="mailto:serguei.spitsyn@oracle.com"><serguei.spitsyn@oracle.com></a> \
<o:p></o:p></pre> <pre>Sent: Freitag, 21. August 2020 11:22<o:p></o:p></pre>
<pre>To: Reingruber, Richard <a \
href="mailto:richard.reingruber@sap.com"><richard.reingruber@sap.com></a>; \
David Holmes <a href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a>; \
<a href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: RFR(S) 8249293: Unsafe stackwalk in \
VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Hi Richard,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thank you for the update, it looks really nice.<o:p></o:p></pre>
<pre>Just several more minor comments though (I hope, the last \
ones).<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Should I rename the variable to spinWaitCycles or something \
similar?<o:p></o:p></pre> </blockquote>
<pre><o:p> </o:p></pre>
<pre>Yes, waitCycles would be better and more consistent.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.udiff. \
html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hotspot/jtre \
g/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.udiff.html</a> \
<o:p></o:p></pre> <pre> 81 * The wait time \
is given in cycles.<o:p></o:p></pre> <pre> 82 \
*/ 83 public int waitTime;<o:p></o:p></pre> <pre> \
...<o:p></o:p></pre> <pre> 93 \
waitTime = 1;<o:p></o:p></pre> <pre>This line 82 can be removed if you rename \
waitTime to waitCycles.<o:p></o:p></pre> <pre>It is better to initialize waitCycles \
at definition and remove the line 93.<o:p></o:p></pre> <pre> \
146 public static void msg(String m) {<o:p></o:p></pre> <pre> \
147 System.out.println("### \
Java-Test: " + m);<o:p></o:p></pre> <pre> 148 \
}<o:p></o:p></pre> <pre>One of the de-facto standard names for such methods is \
"log".<o:p></o:p></pre> <pre> 80 * Wait \
time in native, i.e. with walkable stack, after notifying agent thread to do \
GetLocalObject() call.<o:p></o:p></pre> <pre> \
89 msg("Test how many frames fit \
on the stack by performing recursive calls until StackOverflowError is \
thrown");<o:p></o:p></pre> <pre>Could you, please, reballance the two long lines \
above?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.fram \
es.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/test/hotspot/j \
treg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>There are several spots that can be simplified a little bit:<o:p></o:p></pre>
<pre> 95 jvmtiError result;<o:p></o:p></pre>
<pre> 96 <o:p></o:p></pre>
<pre> 97 result = jvmti->GetErrorName(errCode, \
&errMsg);<o:p></o:p></pre> <pre> ==><o:p></o:p></pre>
<pre> jvmtiError result = jvmti->GetErrorName(errCode, \
&errMsg);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>The same is true for for the following cases:<o:p></o:p></pre>
<pre> 115 err = jvmti->RawMonitorEnter(glws_monitor);<o:p></o:p></pre>
<pre> 125 err = jvmti->RawMonitorExit(glws_monitor);<o:p></o:p></pre>
<pre> 135 err = jvmti->RawMonitorWait(glws_monitor, \
0);<o:p></o:p></pre> <pre> 145 err = \
jvmti->RawMonitorNotify(glws_monitor);<o:p></o:p></pre> <pre> 155 err \
= jvmti->DestroyRawMonitor(glws_monitor);<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre> 173 if (errMsg != NULL) \
{<o:p></o:p></pre> <pre>An extra space before NULL.<o:p></o:p></pre>
<pre> 89 static jvmtiEnv* jvmti_global = NULL;<o:p></o:p></pre>
<pre> 276 jvmtiEnv* jvmti = jvmti_global;<o:p></o:p></pre>
<pre> 308 jvmtiEnv* jvmti = jvmti_global;<o:p></o:p></pre>
<pre> 330 jvmtiEnv* jvmti = jvmti_global;<o:p></o:p></pre>
<pre> ...<o:p></o:p></pre>
<pre> 409 jvmtiEnv* jvmti;<o:p></o:p></pre>
<pre> 419 res = jvm->GetEnv((void **) &jvmti, \
JVMTI_VERSION_9);<o:p></o:p></pre> <pre> 424 jvmti_global = \
jvmti;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Normal practice is to name the "global_jvmti" as \
"jvmti".<o:p></o:p></pre> <pre>Then there is no need to set it at the start \
of each function.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Serguei<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 8/20/20 23:47, Reingruber, Richard wrote:<o:p></o:p></pre>
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Sorry for the delay in reply and thank you for the update.<o:p></o:p></pre>
<pre>I like it in general. There are some minor comments though.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Excellent, thanks :)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I've prepared webrev.5.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Webrev: <a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5/</a><o:p></o:p></pre>
<pre>Delta: <a \
href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.5.inc/</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames \
.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtr \
eg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html</a><o:p></o:p></pre>
<pre> 112 \
waitTime = (waitTime << 1); // double wait time<o:p></o:p></pre> <pre> \
113 if \
(waitTime >= M || waitTime < 0) {<o:p></o:p></pre> <pre> \
114 \
waitTime = 1; // reset when too long<o:p></o:p></pre> \
<pre> 115 \
}<o:p></o:p></pre> <pre>The M is too big for time.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>"waitTime" is roughly the number of cycles spent in a spin wait. M ~= \
10^6<o:p></o:p></pre> <pre>cycles does not seem too long. Should I rename the \
variable to spinWaitCycles or<o:p></o:p></pre> <pre>something \
similar?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>What about something like this:<o:p></o:p></pre>
<pre> waitTime = (waitTime << 1) % 32;<o:p></o:p></pre>
<pre>or<o:p></o:p></pre>
<pre> waitTime = (waitTime << 1) & 32;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I went for<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> // Double \
wait time, but limit to roughly 10^6 cycles.<o:p></o:p></pre> \
<pre> waitTime = \
(waitTime << 1) & (M - 1);<o:p></o:p></pre> \
<pre> waitTime = \
waitTime == 0 ? 1 : waitTime;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Masking the waitTime with % 32 is too small. In my experiments with \
fastdebug<o:p></o:p></pre> <pre>builds I got the crash often with a waitTime of 8K on \
a Linux server and 256K on<o:p></o:p></pre> <pre>my Windows \
notebook.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udif \
f.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jt \
reg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udiff.html</a><o:p></o:p></pre>
<pre>// - Wait for the target thread to either start a new test iteration or \
to<o:p></o:p></pre> <pre>+// signal shutdown.<o:p></o:p></pre>
<pre>A suggestion to replace: "to either start" => "either to \
start". <o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+// Shutdown is signalled by setting test_state to \
ShutDown. The agent reacts<o:p></o:p></pre> <pre>+// to it by \
changing test_state to Terminated and then it exits.<o:p></o:p></pre> <pre>The second \
"it" is not needed: "then it exits" => "then \
exits".<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+// ... It sets the shared variable \
test_state<o:p></o:p></pre> <pre>+// to TargetInNative and \
then it uses the glws_monitor to send the<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre>The second "it" is not needed. \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+ monitor_enter(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre>
<pre>+ monitor_notify(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre>
<pre>+ monitor_wait(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre>+ monitor_exit(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre>+ monitor_destroy(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>There is only one lock.<o:p></o:p></pre>
<pre>It'd be more simple to directly use it in the called functions and get rid of \
the parameter.<o:p></o:p></pre> <pre>Just a suggestion, it is up to you to \
decide.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.fram \
es.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/j \
treg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I'd suggest to refactor the lines 213 and 239-257 to a separate function \
test_GetLocalObject(jvmti, depth).<o:p></o:p></pre> <pre> \
240 jobject \
local_val;<o:p></o:p></pre> <pre>Better to rename it to local_obj or just obj. \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>There are still problems with the indent.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I reformatted the file using 2 space indentation like in other C++ sources. I \
didn't include the<o:p></o:p></pre> <pre>indentation change in the delta \
webrev.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>______________________<o:p></o:p></pre>
<pre>From: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
<a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
<o:p></o:p></pre> <pre>Sent: Donnerstag, 20. August 2020 04:42<o:p></o:p></pre>
<pre>To: Reingruber, Richard <a \
href="mailto:richard.reingruber@sap.com">mailto:richard.reingruber@sap.com</a>; David \
Holmes <a href="mailto:david.holmes@oracle.com">mailto:david.holmes@oracle.com</a>; \
<a href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: RFR(S) 8249293: Unsafe stackwalk in \
VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Hi Richard,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Sorry for the delay in reply and thank you for the update.<o:p></o:p></pre>
<pre>I like it in general. There are some minor comments though.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames \
.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jtr \
eg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html</a><o:p></o:p></pre>
<pre> 112 \
waitTime = (waitTime << 1); // double wait time<o:p></o:p></pre> <pre> \
113 if \
(waitTime >= M || waitTime < 0) {<o:p></o:p></pre> <pre> \
114 \
waitTime = 1; // reset when too long<o:p></o:p></pre> <pre> \
115 \
}<o:p></o:p></pre> <pre>The M is too big for time.<o:p></o:p></pre>
<pre>What about something like this:<o:p></o:p></pre>
<pre> waitTime = (waitTime << 1) % 32;<o:p></o:p></pre>
<pre>or<o:p></o:p></pre>
<pre> waitTime = (waitTime << 1) & 32;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>You can choose a better number instead of 32. <o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udif \
f.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/jt \
reg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.udiff.html</a><o:p></o:p></pre>
<pre>// - Wait for the target thread to either start a new test iteration or \
to<o:p></o:p></pre> <pre>+// signal shutdown.<o:p></o:p></pre>
<pre>A suggestion to replace: "to either start" => "either to \
start". <o:p></o:p></pre> <pre>+// Shutdown is signalled \
by setting test_state to ShutDown. The agent reacts<o:p></o:p></pre> \
<pre>+// to it by changing test_state to Terminated and then \
it exits.<o:p></o:p></pre> <pre>The second "it" is not needed: "then \
it exits" => "then exits".<o:p></o:p></pre> \
<pre>+// ... It sets the shared variable \
test_state<o:p></o:p></pre> <pre>+// to TargetInNative and \
then it uses the glws_monitor to send the<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre>The second "it" is not needed. \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>+ monitor_enter(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre>
<pre>+ monitor_notify(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre>
<pre>+ monitor_wait(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre>+ monitor_exit(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre>+ monitor_destroy(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>There is only one lock.<o:p></o:p></pre>
<pre>It'd be more simple to directly use it in the called functions and get rid of \
the parameter.<o:p></o:p></pre> <pre>Just a suggestion, it is up to you to \
decide.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.fram \
es.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/test/hotspot/j \
treg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I'd suggest to refactor the lines 213 and 239-257 to a separate function \
test_GetLocalObject(jvmti, depth).<o:p></o:p></pre> <pre> \
240 jobject \
local_val;<o:p></o:p></pre> <pre>Better to rename it to local_obj or just obj. \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>There are still problems with the indent.<o:p></o:p></pre>
<pre>The indent 4 is mostly used.<o:p></o:p></pre>
<pre>However there are still fragments with the indent 2:<o:p></o:p></pre>
<pre> 112 static void monitor_enter(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID mon, \
const char* loc) {<o:p></o:p></pre> <pre> 113 jvmtiError \
err;<o:p></o:p></pre> <pre> 114 <o:p></o:p></pre>
<pre> 115 err = jvmti->RawMonitorEnter(mon);<o:p></o:p></pre>
<pre> 116 if (err != JVMTI_ERROR_NONE) {<o:p></o:p></pre>
<pre> 117 ShowErrorMessage(jvmti, err, loc);<o:p></o:p></pre>
<pre> 118 env->FatalError(loc);<o:p></o:p></pre>
<pre> 119 }<o:p></o:p></pre>
<pre> 120 }<o:p></o:p></pre>
<pre> 121 <o:p></o:p></pre>
<pre> 122 static void monitor_exit(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID \
mon, const char* loc) {<o:p></o:p></pre> <pre> 123 jvmtiError \
err;<o:p></o:p></pre> <pre> 124 <o:p></o:p></pre>
<pre> 125 err = jvmti->RawMonitorExit(mon);<o:p></o:p></pre>
<pre> 126 if (err != JVMTI_ERROR_NONE) {<o:p></o:p></pre>
<pre> 127 ShowErrorMessage(jvmti, err, loc);<o:p></o:p></pre>
<pre> 128 env->FatalError(loc);<o:p></o:p></pre>
<pre> 129 }<o:p></o:p></pre>
<pre> 130 }<o:p></o:p></pre>
<pre> 131 <o:p></o:p></pre>
<pre> 132 static void monitor_wait(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID \
mon, const char* loc) {<o:p></o:p></pre> <pre> 133 jvmtiError \
err;<o:p></o:p></pre> <pre> 134 <o:p></o:p></pre>
<pre> 135 err = jvmti->RawMonitorWait(mon, 0);<o:p></o:p></pre>
<pre> 136 if (err != JVMTI_ERROR_NONE) {<o:p></o:p></pre>
<pre> 137 ShowErrorMessage(jvmti, err, loc);<o:p></o:p></pre>
<pre> 138 env->FatalError(loc);<o:p></o:p></pre>
<pre> 139 }<o:p></o:p></pre>
<pre> 140 }<o:p></o:p></pre>
<pre> 141 <o:p></o:p></pre>
<pre> 142 static void monitor_notify(jvmtiEnv* jvmti, JNIEnv* env, jrawMonitorID \
mon, const char* loc) {<o:p></o:p></pre> <pre> 143 jvmtiError \
err;<o:p></o:p></pre> <pre> 144 <o:p></o:p></pre>
<pre> 145 err = jvmti->RawMonitorNotify(mon);<o:p></o:p></pre>
<pre> 146 if (err != JVMTI_ERROR_NONE) {<o:p></o:p></pre>
<pre> 147 ShowErrorMessage(jvmti, err, loc);<o:p></o:p></pre>
<pre> 148 env->FatalError(loc);<o:p></o:p></pre>
<pre> 149 }<o:p></o:p></pre>
<pre> 150 }<o:p></o:p></pre>
<pre> 151 <o:p></o:p></pre>
<pre> 152 static void monitor_destroy(jvmtiEnv* jvmti, JNIEnv* env, \
jrawMonitorID mon, const char* loc) {<o:p></o:p></pre> <pre> 153 \
jvmtiError err;<o:p></o:p></pre> <pre> 154 <o:p></o:p></pre>
<pre> 155 err = jvmti->DestroyRawMonitor(mon);<o:p></o:p></pre>
<pre> 156 if (err != JVMTI_ERROR_NONE) {<o:p></o:p></pre>
<pre> 157 ShowErrorMessage(jvmti, err, loc);<o:p></o:p></pre>
<pre> 158 env->FatalError(loc);<o:p></o:p></pre>
<pre> 159 }<o:p></o:p></pre>
<pre> ...<o:p></o:p></pre>
<pre> 160 }<o:p></o:p></pre>
<pre> 196 while (target_thread == NULL) {<o:p></o:p></pre>
<pre> 197 monitor_wait(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 198 }<o:p></o:p></pre>
<pre> ...<o:p></o:p></pre>
<pre> 220 while (test_state != \
TargetInNative) {<o:p></o:p></pre> <pre> \
221 if (test_state == \
ShutDown) {<o:p></o:p></pre> <pre> \
222 \
test_state = Terminated;<o:p></o:p></pre> <pre> \
223 \
monitor_notify(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre> <pre> \
224 \
monitor_exit(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre> <pre> \
225 \
return;<o:p></o:p></pre> <pre> \
226 }<o:p></o:p></pre> \
<pre> 227 \
monitor_wait(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre> <pre> \
228 }<o:p></o:p></pre> <pre> \
...<o:p></o:p></pre> <pre> 263 // Called by target thread after building a large \
stack.<o:p></o:p></pre> <pre> 264 // By calling this native method, the thread's \
stack becomes walkable.<o:p></o:p></pre> <pre> 265 // It notifies the agent to do the \
GetLocalObject() call and then races<o:p></o:p></pre> <pre> 266 // it to make its \
stack not walkable by returning from the native call.<o:p></o:p></pre> <pre> 267 \
JNIEXPORT void JNICALL<o:p></o:p></pre> <pre> 268 \
Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocal(JNIEnv *env, jclass cls, jint \
depth, jlong waitCycles) {<o:p></o:p></pre> <pre> 269 jvmtiEnv* jvmti = \
jvmti_global;<o:p></o:p></pre> <pre> 270 <o:p></o:p></pre>
<pre> 271 monitor_enter(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 272 <o:p></o:p></pre>
<pre> 273 // Set depth_for_get_local and notify agent that the \
target thread is ready for the GetLocalObject() call<o:p></o:p></pre> <pre> \
274 depth_for_get_local = depth;<o:p></o:p></pre> <pre> 275 \
test_state = TargetInNative;<o:p></o:p></pre> <pre> 276 <o:p></o:p></pre>
<pre> 277 monitor_notify(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 278 <o:p></o:p></pre>
<pre> 279 // Wait for agent thread to read depth_for_get_local and \
do the GetLocalObject() call<o:p></o:p></pre> <pre> 280 while (test_state \
!= AgentInGetLocal) {<o:p></o:p></pre> <pre> 281 \
monitor_wait(jvmti, env, glws_monitor, AT_LINE);<o:p></o:p></pre> <pre> \
282 }<o:p></o:p></pre> <pre> 283 <o:p></o:p></pre>
<pre> 284 // Reset state to Initial<o:p></o:p></pre>
<pre> 285 test_state = Initial;<o:p></o:p></pre>
<pre> 286 <o:p></o:p></pre>
<pre> 287 monitor_exit(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 288 <o:p></o:p></pre>
<pre> 289 // Wait a little until agent thread is in unsafe stack \
walk.<o:p></o:p></pre> <pre> 290 // This needs to be a spin wait or sleep \
because we cannot get a notification<o:p></o:p></pre> <pre> 291 // from \
there.<o:p></o:p></pre> <pre> 292 while (--waitCycles > 0) \
{<o:p></o:p></pre> <pre> 293 \
dummy_counter++;<o:p></o:p></pre> <pre> 294 }<o:p></o:p></pre>
<pre> 295 }<o:p></o:p></pre>
<pre> ...<o:p></o:p></pre>
<pre> 299 JNIEXPORT void JNICALL<o:p></o:p></pre>
<pre> 300 Java_GetLocalWithoutSuspendTest_shutDown(JNIEnv *env, jclass cls) \
{<o:p></o:p></pre> <pre> 301 jvmtiEnv* jvmti = \
jvmti_global;<o:p></o:p></pre> <pre> 302 <o:p></o:p></pre>
<pre> 303 monitor_enter(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 304 <o:p></o:p></pre>
<pre> 305 // Notify agent thread to shut down<o:p></o:p></pre>
<pre> 306 test_state = ShutDown;<o:p></o:p></pre>
<pre> 307 monitor_notify(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 308 <o:p></o:p></pre>
<pre> 309 // Wait for agent to terminate<o:p></o:p></pre>
<pre> 310 while (test_state != Terminated) {<o:p></o:p></pre>
<pre> 311 monitor_wait(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 312 }<o:p></o:p></pre>
<pre> 313 <o:p></o:p></pre>
<pre> 314 monitor_exit(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 315 <o:p></o:p></pre>
<pre> 316 // Destroy glws_monitor<o:p></o:p></pre>
<pre> 317 monitor_destroy(jvmti, env, glws_monitor, \
AT_LINE);<o:p></o:p></pre> <pre> 318 }<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Serguei<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 8/14/20 07:06, Reingruber, Richard wrote:<o:p></o:p></pre>
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>thanks for the feedback. I have implemented your suggestions and created a \
new<o:p></o:p></pre> <pre>webrev:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Webrev: <a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4/</a><o:p></o:p></pre>
<pre>Delta: <a \
href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.4.inc/</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Please find my replies to your comments below.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Best regards,<o:p></o:p></pre>
<pre>Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames \
.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtr \
eg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 33 * the \
stack walk. The target thread's stack is walkable while in native. After sending the \
notification it<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 54 * @param depth Depth of target frame for \
GetLocalObject() call. Should be large value to prolong the unsafe stack \
walk.<o:p></o:p></pre> <pre> 55 * @param \
waitTimeInNativeAfterNotify Time to wait after notify with walkable stack before \
returning an becoming unsafe again.<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 71 * Wait time in native, i.e. with \
walkable stack, after notifying agent thread to do GetLocalObject() \
call.<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 89 \
msg((now -start) + " ms Iteration : " + iterations + " \
waitTimeInNativeAfterNotify : " + waitTimeInNativeAfterNotify);<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre>Could you, please, re-balance the lines above to \
make them shorter?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 90 \
int newTargetDepth = recursiveMethod(0, targetDepth);<o:p></o:p></pre> <pre> \
91 if \
(newTargetDepth < targetDepth) {<o:p></o:p></pre> <pre> \
92 \
msg("StackOverflowError during test.");<o:p></o:p></pre> <pre> \
93 \
msg("Old target depth: " + targetDepth);<o:p></o:p></pre> <pre> \
94 \
msg("Retry with new target depth: " + newTargetDepth);<o:p></o:p></pre> \
<pre> 95 \
targetDepth = newTargetDepth;<o:p></o:p></pre> <pre> \
96 \
}<o:p></o:p></pre> <pre>A comment is needed to explain why a StackOverflowError is \
not desired.<o:p></o:p></pre> <pre>At least, it is not obvious \
initially.<o:p></o:p></pre> <pre> 73 public int \
waitTimeInNativeAfterNotify;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>This name is unreasonably long which makes the code less \
readable.<o:p></o:p></pre> <pre>I'd suggest to reduce it to \
waitTime.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 103 \
notifyAgentToGetLocalAndWaitShortly(-1, 1);<o:p></o:p></pre> <pre> \
...<o:p></o:p></pre> <pre> \
119 \
notifyAgentToGetLocalAndWaitShortly(depth - 100, \
waitTimeInNativeAfterNotify);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is better to provide a short comment before each call explaining what it is \
doing.<o:p></o:p></pre> <pre>For instance, it is not clear why the call at the line \
103 is needed.<o:p></o:p></pre> <pre>Why do we need to notify the agent to GetLocal \
for the second time?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>The test is repeated TEST_ITERATIONS times. In each iteration the agent \
calls<o:p></o:p></pre> <pre>GetLocal racing the target thread returning from the \
native call. The last call<o:p></o:p></pre> <pre>in line 103 ist the shutdown \
signal.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Can it be refactored into a separate native method?<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I've made the shutdown process more explicit with the new native \
method<o:p></o:p></pre> <pre>shutDown() which sets thest_state to \
ShutDown.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Then the the function name can be reduced to \
'notifyAgentToGetLocal'.<o:p></o:p></pre> <pre>This long name does not give enough \
context anyway.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 85 long iterations = \
0;<o:p></o:p></pre> <pre> 87 do \
{<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre> 97 \
iterations++;<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre> 102 } while (iterations < \
TEST_ITERATIONS);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Why a more explicit 'for' or 'while' loop is not used here? :<o:p></o:p></pre>
<pre>for (long iter = 0; iter < TEST_ITERATIONS; iter++) { <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>I have converted the loop into a for loop.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.fram \
es.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/j \
treg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The indent in this file varies. It is better to keep it the same: 4 or \
2.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Yes, I noticed this. I have not corrected it yet, because I didn't want \
to<o:p></o:p></pre> <pre>pullute the incremental webrev with that change. Would you \
like me to fix the<o:p></o:p></pre> <pre>indentation now to 2 spaces or do it as a \
last step?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre> 60 AgentCallingGetLocalObject // The target thread waits for \
the agent to call<o:p></o:p></pre> <pre>I'd suggest to rename the constant to \
'AgentInGetLocal'.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 150 GetLocalWithoutSuspendTestThreadLoop(jvmtiEnv * jvmti, JNIEnv* env, void * \
arg) {<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is better rename the function to TestThreadLoop.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Would AgentThreadLoop be ok too?<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>You can add a comment before to explain some basic about what it is \
doing.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 167 printf("*** AGENT: \
GetLocalWithoutSuspendTestThreadLoop thread started. Polling thread '%s' for local \
variables\n",<o:p></o:p></pre> <pre>It is better to get rid of leading stars in \
all messages.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 176 // the native method \
Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocalAndWaitShortly<o:p></o:p></pre> \
<pre>The part 'Java_GetLocalWithoutSuspendTest_' can be removed from the function \
name.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Ok, done.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>---<o:p></o:p></pre>
<pre>From: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
<a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
<o:p></o:p></pre> <pre>Sent: Freitag, 14. August 2020 10:11<o:p></o:p></pre>
<pre>To: Reingruber, Richard <a \
href="mailto:richard.reingruber@sap.com">mailto:richard.reingruber@sap.com</a>; David \
Holmes <a href="mailto:david.holmes@oracle.com">mailto:david.holmes@oracle.com</a>; \
<a href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: RFR(S) 8249293: Unsafe stackwalk in \
VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Hi Richard,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames \
.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/jtr \
eg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 33 * the \
stack walk. The target thread's stack is walkable while in native. After sending the \
notification it<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 54 * @param depth Depth of target frame for \
GetLocalObject() call. Should be large value to prolong the unsafe stack \
walk.<o:p></o:p></pre> <pre> 55 * @param \
waitTimeInNativeAfterNotify Time to wait after notify with walkable stack before \
returning an becoming unsafe again.<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 71 * Wait time in native, i.e. with \
walkable stack, after notifying agent thread to do GetLocalObject() \
call.<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 89 \
msg((now -start) + " ms Iteration : " + iterations + " \
waitTimeInNativeAfterNotify : " + waitTimeInNativeAfterNotify);<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre>Could you, please, re-balance the lines above to \
make them shorter?<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 90 \
int newTargetDepth = recursiveMethod(0, targetDepth);<o:p></o:p></pre> <pre> \
91 if \
(newTargetDepth < targetDepth) {<o:p></o:p></pre> <pre> \
92 \
msg("StackOverflowError during test.");<o:p></o:p></pre> <pre> \
93 \
msg("Old target depth: " + targetDepth);<o:p></o:p></pre> <pre> \
94 \
msg("Retry with new target depth: " + newTargetDepth);<o:p></o:p></pre> \
<pre> 95 \
targetDepth = newTargetDepth;<o:p></o:p></pre> <pre> \
96 \
}<o:p></o:p></pre> <pre>A comment is needed to explain why a StackOverflowError is \
not desired.<o:p></o:p></pre> <pre>At least, it is not obvious \
initially.<o:p></o:p></pre> <pre> 73 public int \
waitTimeInNativeAfterNotify;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>This name is unreasonably long which makes the code less \
readable.<o:p></o:p></pre> <pre>I'd suggest to reduce it to \
waitTime.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre> 103 \
notifyAgentToGetLocalAndWaitShortly(-1, 1);<o:p></o:p></pre> <pre> \
...<o:p></o:p></pre> <pre> \
119 \
notifyAgentToGetLocalAndWaitShortly(depth - 100, \
waitTimeInNativeAfterNotify);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is better to provide a short comment before each call explaining what it is \
doing.<o:p></o:p></pre> <pre>For instance, it is not clear why the call at the line \
103 is needed.<o:p></o:p></pre> <pre>Why do we need to notify the agent to GetLocal \
for the second time?<o:p></o:p></pre> <pre>Can it be refactored into a separate \
native method?<o:p></o:p></pre> <pre>Then the the function name can be reduced to \
'notifyAgentToGetLocal'.<o:p></o:p></pre> <pre>This long name does not give enough \
context anyway.<o:p></o:p></pre> <pre> \
85 long iterations = \
0;<o:p></o:p></pre> <pre> 87 do \
{<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre> 97 \
iterations++;<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre> 102 } while (iterations < \
TEST_ITERATIONS);<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Why a more explicit 'for' or 'while' loop is not used here? :<o:p></o:p></pre>
<pre>for (long iter = 0; iter < TEST_ITERATIONS; iter++) { <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hots \
pot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.fram \
es.html">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/test/hotspot/j \
treg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp.frames.html</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The indent in this file varies. It is better to keep it the same: 4 or 2. \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre> 60 AgentCallingGetLocalObject // The target thread waits for \
the agent to call<o:p></o:p></pre> <pre>I'd suggest to rename the constant to \
'AgentInGetLocal'.<o:p></o:p></pre> <pre> 150 \
GetLocalWithoutSuspendTestThreadLoop(jvmtiEnv * jvmti, JNIEnv* env, void * arg) \
{<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is better rename the function to TestThreadLoop.<o:p></o:p></pre>
<pre>You can add a comment before to explain some basic about what it is doing. \
<o:p></o:p></pre> <pre> 167 printf("*** AGENT: \
GetLocalWithoutSuspendTestThreadLoop thread started. Polling thread '%s' for local \
variables\n",<o:p></o:p></pre> <pre>It is better to get rid of leading stars in \
all messages.<o:p></o:p></pre> <pre> \
176 // the native method \
Java_GetLocalWithoutSuspendTest_notifyAgentToGetLocalAndWaitShortly<o:p></o:p></pre> \
<pre>The part 'Java_GetLocalWithoutSuspendTest_' can be removed from the function \
name.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I'm still reviewing the test native agent code.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Serguei<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 8/11/20 03:02, Reingruber, Richard wrote:<o:p></o:p></pre>
<pre>Hi David and Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 11/08/2020 3:21 am, <a \
href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
wrote:<o:p></o:p></pre> <pre>Hi Richard and David,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The implementation looks good to me.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>But I do not understand what the test is doing with all this counters \
<o:p></o:p></pre> <pre>and recursions.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>For instance, these fragments:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 86 recursions = \
0;<o:p></o:p></pre> <pre> \
87 try {<o:p></o:p></pre> \
<pre> 88 \
recursiveMethod(1<<20);<o:p></o:p></pre> <pre> \
89 } catch (StackOverflowError e) \
{<o:p></o:p></pre> <pre> \
90 \
msg("Caught StackOverflowError as expected");<o:p></o:p></pre> \
<pre> 91 \
}<o:p></o:p></pre> <pre> \
92 int target_depth = \
recursions-100; // spaces are missed around the '-' \
sigh<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is not obvious that the 'recursion' is updated in the \
recursiveMethod.<o:p></o:p></pre> <pre>I would suggestto make it more \
explicit:<o:p></o:p></pre> <pre> \
recursiveMethod(M);<o:p></o:p></pre> <pre> int target_depth = M - \
100;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Then the variable 'recursions' can be removed or become local.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The recursiveMethod takes in the maximum recursions to try and updates \
<o:p></o:p></pre> <pre>the recursions variable to record how many recursions were \
possible - so:<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>target_depth = <actual recursions> - 100;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Possibly recursiveMethod could return the actual recursions instead of \
<o:p></o:p></pre> <pre>using the global variables?<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I've eliminated the static 'recursions' variable. recursiveMethod() now \
returns<o:p></o:p></pre> <pre>the depth at which the recursion was ended. I hesitated \
doing this, because I<o:p></o:p></pre> <pre>had to handle the StackOverflowError with \
all those frames still on stack. But<o:p></o:p></pre> <pre>the handler is empty, so \
it should not cause problems.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>This is the new webrev (as posted previously):<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Webrev: <a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3/</a><o:p></o:p></pre>
<pre>Delta: <a \
href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.3.inc/</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks, Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: David Holmes <a \
href="mailto:david.holmes@oracle.com">mailto:david.holmes@oracle.com</a> \
<o:p></o:p></pre> <pre>Sent: Dienstag, 11. August 2020 04:00<o:p></o:p></pre>
<pre>To: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a>; \
Reingruber, Richard <a \
href="mailto:richard.reingruber@sap.com">mailto:richard.reingruber@sap.com</a>; <a \
href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: RFR(S) 8249293: Unsafe stackwalk in \
VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 11/08/2020 3:21 am, <a \
href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
wrote:<o:p></o:p></pre> <pre>Hi Richard and David,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The implementation looks good to me.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>But I do not understand what the test is doing with all this counters \
<o:p></o:p></pre> <pre>and recursions.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>For instance, these fragments:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 86 recursions = \
0;<o:p></o:p></pre> <pre> \
87 try {<o:p></o:p></pre> \
<pre> 88 \
recursiveMethod(1<<20);<o:p></o:p></pre> <pre> \
89 } catch (StackOverflowError e) \
{<o:p></o:p></pre> <pre> \
90 \
msg("Caught StackOverflowError as expected");<o:p></o:p></pre> \
<pre> 91 \
}<o:p></o:p></pre> <pre> \
92 int target_depth = \
recursions-100; // spaces are missed around the '-' \
sigh<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>It is not obvious that the 'recursion' is updated in the \
recursiveMethod.<o:p></o:p></pre> <pre>I would suggestto make it more \
explicit:<o:p></o:p></pre> <pre> recursiveMethod(M);<o:p></o:p></pre>
<pre> int target_depth = M - 100;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Then the variable 'recursions' can be removed or become local.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The recursiveMethod takes in the maximum recursions to try and updates \
<o:p></o:p></pre> <pre>the recursions variable to record how many recursions were \
possible - so:<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>target_depth = <actual recursions> - 100;<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Possibly recursiveMethod could return the actual recursions instead of \
<o:p></o:p></pre> <pre>using the global variables?<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>David<o:p></o:p></pre>
<pre>-----<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>This method will be:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 47 private static final int M = 1 << \
20;<o:p></o:p></pre> <pre> ...<o:p></o:p></pre>
<pre> 121 public long recursiveMethod(int depth) \
{<o:p></o:p></pre> <pre> 123 if \
(depth == 0) {<o:p></o:p></pre> <pre> \
124 \
notifyAgentToGetLocalAndWaitShortly(M - 100, \
waitTimeInNativeAfterNotify);<o:p></o:p></pre> <pre> \
126 } else {<o:p></o:p></pre> \
<pre> 127 \
recursiveMethod(--depth);<o:p></o:p></pre> <pre> \
128 }<o:p></o:p></pre> <pre> \
129 }<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>At least, he test is missing the comments explaining all these.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Serguei<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 8/9/20 22:35, David Holmes wrote:<o:p></o:p></pre>
<pre>Hi Richard,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 31/07/2020 5:28 pm, Reingruber, Richard wrote:<o:p></o:p></pre>
<pre>Hi,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I rebase the fix after JDK-8250042.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>New webrev: <a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.2/" \
>http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.2/</a><o:p></o:p></pre> \
> <pre><o:p> </o:p></pre>
<pre>The general fix for this seems good. A minor nit:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 588 if (!is_assignable(signature, ob_k, \
Thread::current())) {<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>You know that the current thread is the VMThread so can use <o:p></o:p></pre>
<pre>VMThread::vm_thread().<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Similarly for this existing code:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 694 Thread* current_thread = \
Thread::current();<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>---<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Looking at the test code ... I'm less clear on exactly what is <o:p></o:p></pre>
<pre>happening and the use of spin-waits raises some red-flags for me in \
<o:p></o:p></pre> <pre>terms of test reliability on different platforms. The \
"while <o:p></o:p></pre> <pre>(--waitCycles > 0)" loop in particular \
offers no certainty that the <o:p></o:p></pre> <pre>agent thread is executing \
anything in particular. And the use of the <o:p></o:p></pre> <pre>spin_count as a \
guide to future waiting time seems somewhat arbitrary. <o:p></o:p></pre> <pre>In all \
seriousness I got a headache trying to work out how the test <o:p></o:p></pre> \
<pre>was expecting to operate. Some parts could be simplified using raw \
<o:p></o:p></pre> <pre>monitors, I think. But there's no sure way to know the agent \
thread is <o:p></o:p></pre> <pre>in the midst of the stackwalk when the target thread \
wants to leave <o:p></o:p></pre> <pre>the native code. So I understand what you are \
trying to achieve here, <o:p></o:p></pre> <pre>I'm just not sure how reliably it will \
actually achieve it.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalWithoutSuspendTest.cpp \
<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 32 static volatile jlong spinn_count = \
0;<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Using a 64-bit counter seems like it will be a problem on 32-bit \
systems.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Should be spin_count not spinn_count.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> 36 // Agent thread waits for value != 0, then performas the JVMTI \
<o:p></o:p></pre> <pre>call to get local variable.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>typo: performas<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>David<o:p></o:p></pre>
<pre>-----<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks, Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: serviceability-dev <a \
href="mailto:serviceability-dev-retn@openjdk.java.net">mailto:serviceability-dev-retn@openjdk.java.net</a> \
<o:p></o:p></pre> <pre>On Behalf Of Reingruber, Richard<o:p></o:p></pre>
<pre>Sent: Montag, 27. Juli 2020 09:45<o:p></o:p></pre>
<pre>To: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a>; \
<a href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: [CAUTION] RE: RFR(S) 8249293: Unsafe stackwalk in <o:p></o:p></pre>
<pre>VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> > I tested it on Linux and Windows but not yet on \
MacOS.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>The test succeeded now on all platforms.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks, Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: Reingruber, Richard<o:p></o:p></pre>
<pre>Sent: Freitag, 24. Juli 2020 15:04<o:p></o:p></pre>
<pre>To: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a>; \
<a href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: RE: RFR(S) 8249293: Unsafe stackwalk in <o:p></o:p></pre>
<pre>VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Hi Serguei,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The fix itself looks good to me.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>thanks for looking at the fix.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I still need another look at new test.<o:p></o:p></pre>
<pre>Could you, please, convert the agent of new test to C++?<o:p></o:p></pre>
<pre>It will make it a little bit simpler.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Sure, here is the new webrev.1 with a C++ version of the test \
agent:<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.1/">http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.1/</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>I tested it on Linux and Windows but not yet on MacOS.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: <a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a> \
<a href="mailto:serguei.spitsyn@oracle.com">mailto:serguei.spitsyn@oracle.com</a><o:p></o:p></pre>
<pre>Sent: Freitag, 24. Juli 2020 00:00<o:p></o:p></pre>
<pre>To: Reingruber, Richard <a \
href="mailto:richard.reingruber@sap.com">mailto:richard.reingruber@sap.com</a>; \
<o:p></o:p></pre> <pre><a \
href="mailto:serviceability-dev@openjdk.java.net">mailto:serviceability-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: RFR(S) 8249293: Unsafe stackwalk in <o:p></o:p></pre>
<pre>VM_GetOrSetLocal::doit_prologue()<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Hi Richard,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thank you for filing the CR and taking care about it!<o:p></o:p></pre>
<pre>The fix itself looks good to me.<o:p></o:p></pre>
<pre>I still need another look at new test.<o:p></o:p></pre>
<pre>Could you, please, convert the agent of new test to C++?<o:p></o:p></pre>
<pre>It will make it a little bit simpler.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre>Serguei<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>On 7/20/20 01:15, Reingruber, Richard wrote:<o:p></o:p></pre>
<pre>Hi,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>please help review this fix for VM_GetOrSetLocal. It moves the <o:p></o:p></pre>
<pre>unsafe stackwalk from the vm<o:p></o:p></pre>
<pre>operation prologue before the safepoint into the doit() method <o:p></o:p></pre>
<pre>executed at the safepoint.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Webrev: <o:p></o:p></pre>
<pre><a href="http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.0/index.html">h \
ttp://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.0/index.html</a><o:p></o:p></pre>
<pre>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8249293">https://bugs.openjdk.java.net/browse/JDK-8249293</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>According to the JVMTI spec on local variable access it is not <o:p></o:p></pre>
<pre>required to suspend the target thread<o:p></o:p></pre>
<pre>T [1]. The operation will simply fail with <o:p></o:p></pre>
<pre>JVMTI_ERROR_NO_MORE_FRAMES if T is executing<o:p></o:p></pre>
<pre>bytecodes. It will succeed though if T is blocked because of <o:p></o:p></pre>
<pre>synchronization or executing some native<o:p></o:p></pre>
<pre>code.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The issue is that in the latter case the stack walk in <o:p></o:p></pre>
<pre>VM_GetOrSetLocal::doit_prologue() to prepare<o:p></o:p></pre>
<pre>the access to the local variable is unsafe, because it is done <o:p></o:p></pre>
<pre>before the safepoint and it races<o:p></o:p></pre>
<pre>with T returning to execute bytecodes making its stack not walkable. \
<o:p></o:p></pre> <pre>The included test shows that<o:p></o:p></pre>
<pre>this can crash the VM if T wins the race.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Manual testing:<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> - new test <o:p></o:p></pre>
<pre>test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java<o:p></o:p></pre>
<pre> - test/hotspot/jtreg/vmTestbase/nsk/jvmti<o:p></o:p></pre>
<pre> - test/hotspot/jtreg/serviceability/jvmti<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Nightly regression tests @SAP: JCK and JTREG, also in Xcomp mode, \
<o:p></o:p></pre> <pre>SPECjvm2008, SPECjbb2015,<o:p></o:p></pre>
<pre>Renaissance Suite, SAP specific tests with fastdebug and release \
<o:p></o:p></pre> <pre>builds on all platforms<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Thanks, Richard.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>[1] <o:p></o:p></pre>
<pre><a href="https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#local">h \
ttps://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#local</a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic