Re: 12 RFR(M) 8195635: [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"

Mon, 28 Jan 2019 22:49:56 -0800

Hi Dean,

I like this approach in general, and think, it is the best to take.
(I already mentioned in the bug report comments).

But there is some risk of regressions as the fix has pretty wide impact.

If I understand correctly, this fix with SuspendableMark helper objects
defines the non-suspendable points (where suspend is not allowed) and also
explicit suspendable points (where suspend is allowed to happen).
By default (in contexts where there is no SuspendableMark) suspend is allowed.

One concern is if there can be some execution contexts with nested opposite SuspendableMark's.
I wonder, if we could add some asserted traps to make sure there are no such cases.
Otherwise, this mechanism can fail to always work correctly.

Also, there is a couple of spots in this fix which I don't fully understand.

One place is in the java_suspend() and java_suspend_self():
   http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/runtime/thread.cpp.frames.html

The fix adds some comments but they are not enough.
For instance, the use of ThreadSuspendClosure and Handshake::execute() is not clear
in the newly added fragment (replaced the use of VM_ThreadSuspend helper object):

2342       if (this == JavaThread::current()) {
2343         return;
2344       }
2345 
2346       // In the past, forcing a safepoint here was enough, but with
2347       // allow_suspend(), not all safepoints are suspend points.
2348       if (thread_state() == _thread_in_Java) {
2349         ThreadSuspendClosure tsc;
2350         Handshake::execute(&tsc, this);
2351       } else {
2352         MutexLockerEx ml(SR_lock(), Mutex::_no_safepoint_check_flag);
2353         // Wait for self-suspend or resume.
2354         // Not all interesting transitions perform a notify, so wait with
2355         // timeout.
2356         SR_lock()->wait(!Mutex::_no_safepoint_check_flag, 10);
2357       }
2358   }

Another place is:
  http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html

It is not easy to understand the consequences of change in the JvmtiEnv::SuspendThreadList().

So, we need our Suspend/Resume experts (Dan and David) to look at this fix.


I also have a couple of minor comments.

 http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html

  Now we have three spots with the same checks:
 936 static jvmtiError suspendThread_request(JavaThread* java_thread) {
 937   // don't allow hidden thread suspend request.
 938   if (java_thread->is_hidden_from_external_view()) {
 939     return (JVMTI_ERROR_NONE);
 940   }
 . . .
 955 static jvmtiError suspendThread_complete(JavaThread* java_thread) {
 956   // don't allow hidden thread suspend request.
 957   if (java_thread->is_hidden_from_external_view()) {
 958     return (JVMTI_ERROR_NONE);
 959   }
 . . .
 970 JvmtiEnv::SuspendThread(JavaThread* java_thread) {
 971   // don't allow hidden thread suspend request.
 972   if (java_thread->is_hidden_from_external_view()) {
 973     return (JVMTI_ERROR_NONE);
 974   }
 At least, the checks in the suspendThread_request() and suspendThread_complete()
 can be moved into the SuspendThreadList() loops to make them more clear.


Also, the SuspendableMark helper objects for non-suspendable points are named differently.
In the jvmciRuntime.cpp it is named 'nosuspend'.
In the sharedRuntime.cpp they are named 'block_suspend'.
This naming can be unified.


Otherwise, it is very good fix to have.
Thank you a lot for taking care about this hard-to-fix issue!

Thanks,
Serguei


On 1/28/19 17:13, [email protected] wrote:
http://cr.openjdk.java.net/~dlong/8195635/webrev.5/
https://bugs.openjdk.java.net/browse/JDK-8195635

Please see the bug report for all the gory details.  Here's the short version:

If we allow any safepoint to be a suspend point, we run into trouble with PopFrame and ForceEarlyReturn, which reasonably expect the top frame not to change between the suspend and when the PopFrame/ForceEarlyReturn is executed.  Normally this is not an issue, but certain safepoints cause problems, when we are about to call a new Java method.  In particular, if we safepoint and suspend in JavaCallWrapper, the top frame will still be the caller, but when we execute the PopFrame/ForceEarlyReturn we will be in the callee.

The solution this patch takes is to block suspend around troublesome VM code using a new "allow_suspend" thread flag.  This means JavaThread::java_suspend can't just ask the VMThread to safepoint and be done.  Instead it has wait and allow threads to roll forward to an allowed suspend point.

dl

Reply via email to