RFR (XS) 6471769: Error: assert(_cur_stack_depth == count_frames(), "cur_stack_depth out of sync")

[Daniel D. Daugherty]

On 1/31/14 7:58 PM, serguei.spitsyn at oracle.com wrote:
> Please, review the fix for:
>   https://bugs.openjdk.java.net/browse/JDK-6471769
>
>
> Open webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-FRAME/ 
>

src/share/vm/prims/jvmtiEnv.cpp
     Thumbs up on the code change.

     I've never been a fan of the:

         if (is_thread_fully_suspended(...))
             do-something-direct
         else
             do-something-via-a-safepoint

     model. We don't have any mechanism in the VM to keep the
     is_thread_fully_suspended() constraint true over the
     execution time of the "do-something-direct" operation so
     we've always been exposed to races. Also, I think this
     model is used for several other JVM/TI calls so you might
     want to visit those also with an eye on safety.

     For this specific change, I think the comment would be
     better as:

     // It is only safe to perform the direct operation on the current
     // thread. All other usage needs to use a vm-op for safety.

     In particular, I don't agree with this part of the comment:

     // Performance-wise the only important case is when current thread is
     // the target thread.

     I'm not sure how you can state that performance on the current thread
     is the only important case, but maybe I don't understand what you're
     really trying to say here.

Now for the more general question with respect to this bug: 6471769
I've having trouble connecting this change (which I like) to this
particular bug report.

The crash in the bug's description looks like this:

# Internal Error 
(/net/prt-solamd64-q1-2/PrtBuildDir/workspace/src/share/vm/prims/jvmtiThreadState.cpp, 
273), pid=9561, tid=2
#
# Java VM: Java HotSpot(TM) 64-Bit Server VM 
(20060914135846.dcubed.service_hs_b02_merge.2-debug compiled mode)
#
# Error: assert(_cur_stack_depth == count_frames(),"cur_stack_depth out 
of sync")

V [libjvm.so+0x1217206];; void VMError::report_and_die()+0x606
V [libjvm.so+0x667841];; void report_assertion_failure(const 
char*,int,const char*)+0x61
V [libjvm.so+0xca9557];; int JvmtiThreadState::cur_stack_depth()+0x6e7
V [libjvm.so+0xc1caaa];; void 
JvmtiExport::post_method_exit(JavaThread*,methodOop,frame)+0x21ba
V [libjvm.so+0x8385de];; void 
InterpreterRuntime::post_method_exit(JavaThread*)+0x21e
j java.io.ObjectInputStream$BlockDataInputStream.getBlockDataMode()Z+4

In the above crash, the current thread is trying to post a
JVMTI_EVENT_METHOD_EXIT and during the event posting code, it
calls cur_stack_depth() which fails an assert because the
cached stack depth doesn't match the current queried value:

    282  int JvmtiThreadState::cur_stack_depth() {
    283    uint32_t debug_bits = 0;
    284    guarantee(JavaThread::current() == get_thread() ||
    285      JvmtiEnv::is_thread_fully_suspended(get_thread(), false, 
&debug_bits
),
    286      "must be current thread or suspended");
    287
    288    if (!is_interp_only_mode() || _cur_stack_depth == 
UNKNOWN_STACK_DEPTH)
  {
    289      _cur_stack_depth = count_frames();
    290    } else {
    291      // heavy weight assert
    292      assert(_cur_stack_depth == count_frames(),
    293             "cur_stack_depth out of sync");
    294    }
    295    return _cur_stack_depth;
    296  }


This is not an unsafe query from one thread to another target thread
so I don't see how the proposed change will address this failure mode.
In particular, the assertion is troubling because it tells me that
the cached stack depth for the current thread is either wrong or has
been corrupted.

To reiterate:

- I like the code change!
- I would like to see the comment tweaked a bit

You should consider going forward with this change using a different
bug ID, perhaps something like:

    JDK-NNNNNNN improve safety of JVM/TI GetFrameCount()

Dan



>
> Summary:
>
>   There is a general issue in the suspend equivalent condition mechanism:
>   Two subsequent calls to the JvmtiEnv::is_thread_fully_suspended() 
> may return different results:
>     - 1-st: true
>     - 2-nd: false
>
>   This more generic suspend equivalent issue is covered by another bug:
>     https://bugs.openjdk.java.net/browse/JDK-6280037
>
>   The bug to fix in this review is a specific manifestation of the 
> 6280037
>   in the JVMTI GetFrameCount() that has a big impact on the SQE nightly.
>   It is on the Test Stabilization radar (as well as the 6280037).
>   There are many tests intermittently failing because of this.
>
>   The webrev for review is a one-liner work around the 6280037 for the 
> GetFrameCount().
>
>   The JVMTI GetFrameCount() spec tells:
>     "If this function is called for a thread actively executing 
> bytecodes (for example,
>      not the current thread and not suspended), the information 
> returned is transient."
>
>   So, it is Ok to call the GetFrameCount() for non-suspended target 
> threads.
>   To achieve safety, the frame count for non-suspended threads is 
> calculated at a safepoint.
>   It should be Ok and more safe to do the same for suspended threads 
> as well.
>   There is no big performance impact because it is already on a slow 
> path.
>   It is still important to avoid safepointing when the target thread 
> is current.
>
>   The bug 6280037 should go out of the Test Stabilization radar 
> (remove the svc-nightly label)
>   as the most of the impacted tests are covered by the 6471769.
>
>
> Testing:
>   In progress: nsk.jvmti, nsk.jdi, nsk.jdwp, impacted JTreg tests
>
>
> Thanks,
> Serguei
>