RFR: 8225351(13): assert failed: Revoke failed, unhandled biased lock state

[Daniel D. Daugherty]

Hi Robbin,

Okay so I read this email and then the bug report. The conclusion sounds
like we have an assert() that is a little too aggressive so I moved on to
the webrev... I expected to find a refinement to an assert(), but that's
not quite what I see...

Let me re-read everything again...

Hmmm... still reads the same, but let me dive into the code instead...


On 6/14/19 4:03 AM, Robbin Ehn wrote:
> Hi all, please review.
>
> When looking at a JavaThreads locks we retrieve them per frame via 
> it's monitors
> list. How this list actually populated differs from frame type. If a 
> JavaThread tries to enter a new monitor a basic lock is 
> directly/indirectly via e.g. scope info added to the virtual monitor 
> list. If this lock is biased towards another
> JavaThread we try to revoke that bias with a safepoint. In this case a 
> deopt
> handshake is already in queue. The handshake is thus executed before 
> the revoke
> safepoint.
> The handshake goes over the monitors in compiled frames, find this 
> lock and we
> hit the assert. The assert make sure we actual can revoke the lock. A 
> basic lock
> on stack should always, if biased, be biased to current thread, with the
> exception:
> We may have a stack lock biased against another thread until
> ObjectSynchronizer::fast_enter returns.
>
> To handle this exception we can safely ignore biased lock towards 
> other threads
> in the deopt handshake. Since such locks will always be revoked before we
> deopt/unpack stack.
>
> Code:
> http://cr.openjdk.java.net/~rehn/8225351/v1/webrev/index.html

First a little analysis:

Our failing assert is here:

src/hotspot/share/runtime/biasedLocking.cpp:

BiasedLocking::Condition 
BiasedLocking::revoke_own_locks_in_handshake(Handle obj, TRAPS) {

   assert(mark->biased_locker() == THREAD &&
          prototype_header->bias_epoch() == mark->bias_epoch(), "Revoke 
failed, unhandled biased lock state");

Since the function is called revoke_own_locks_in_handshake() it makes
sense that we're checking that the locks are biased to THREAD (presumably
the current thread since that's the TRAPS protocol). Just checking...
Uhhhh... Looks like we can get to this code path from
Deoptimization::deoptimize_frame_internal() when the current thread is
the target thread *AND* when we are deoptimizing at a safepoint and the
current thread is not the target thread. To my brain that makes use of
the 'TRAPS' parameter "wrong", i.e., it should a "Thread* thread"
parameter, but that's probably a carry over from older biased locking
code.

However, let's leave the topic of the "TRAPS" parameter alone for a
separate investigation. It's entirely possible that my memory about the
TRAPS protocol is rusty (or it has evolved since I last committed it to
memory).


src/hotspot/share/runtime/deoptimization.cpp
     old L1464:     oop obj = (objects_to_revoke->at(i))();
     old L1465: 
BiasedLocking::revoke_own_locks_in_handshake(objects_to_revoke->at(i), 
thread);

     new L1486:     Handle obj = objects_to_revoke->at(i);
     new L1487:     markOop mark = obj->mark();
     new L1488:     if (mark->has_bias_pattern()) {
     new L1489:       if (mark->biased_locker() == thread) {
     new L1490: BiasedLocking::revoke_own_locks_in_handshake(obj, thread);

         So BiasedLocking::revoke_own_locks_in_handshake() has:

         >  markOop mark = obj->mark();
         >  if (!mark->has_bias_pattern()) {
         >    return NOT_BIASED;
         >  }

         which shows that the code previously recognized that it could be
         called with an 'obj' parameter that wasn't biased. Perhaps it
         would be better to change revoke_own_locks_in_handshake() to
         something like:

         if (!mark->has_bias_pattern() || mark->biased_locker() != THREAD) {

         Of course, that means we would be returning "NOT_BIASED" for
         the case where the target object _is_ biased, but just not
         biased to our target thread.

         Another thought occurs to me: If this is happening during a 
handshake
         and not at a safepoint, then what prevents the object from passing
         new L1488 and L1489 in deoptimization.cpp, then we call
         revoke_own_locks_in_handshake() and pass 
"(!mark->has_bias_pattern())"
         only discover that the object has been rebiased to another 
thread at
         the assert? In other words, what happens if this revocation attempt
         loses a race with another revocation & rebias attempt?

     L1493:         #ifdef ASSERT
     L1494:         lock_in_top_frame(obj, thread);
     L1495:         #endif
         I think this should be:

                     DEBUG_ONLY(lock_in_top_frame(obj, thread);)

     L1456: void lock_in_top_frame(Handle ho, JavaThread* thread) {
     L1463:     if (mon_info->eliminated()) {
     L1464:       continue;
     L1465:     }
     L1473:   assert(false, "Lock not found in top frame");
         I'm having problems with this sanity check. That function expects
         to find a BasicLock (via a MonitorInfo) that refers to the
         object whose bias we are trying to revoke. That check is assuming
         that a biased lock must be in the thread's top frame and I don't
         think that is a valid assumption.

         revoke_using_handshake() calls get_monitors_from_stack() and that
         function collects objects that have monitors from all vframes into
         'objects_to_revoke'.

         Maybe I'm missing something obvious here?

Dan

> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8225351
>
> Passes t1-7
> The assert code tested with local code changes to HandshakeAlot 
> handshake.
> We then see this state where last lock can be biased towards another 
> thread and the thread is trying to execute revoke safepoint.
>
> Thanks, Robbin