RFR(S): 8229212 clear up CHECK_OWNER confusion in objectMonitor.cpp

[David Holmes]

Hi Dan,

I see what you are doing but I found things a little confusing ...

On 9/08/2019 12:25 am, Daniel D. Daugherty wrote:
> Greetings,
> 
> Discussions with David Holmes about ObjectMonitors revealed a bit of
> a mess with the CHECK_OWNER macro and the ObjectMonitor::check() and
> ObjectMonitor::check_slow() functions. I have a small fix to clean up
> this bit of messy code.
> 
> The bug URL:
> 
>      JDK-8229212 clear up CHECK_OWNER confusion in objectMonitor.cpp
>      https://bugs.openjdk.java.net/browse/JDK-8229212
> 
> The webrev URL:
> 
>      http://cr.openjdk.java.net/~dcubed/8229212-webrev/0_for_jdk14/
> 
> Summary of the fix:
> 
>      Merge the CHECK_OWNER macro and the ObjectMonitor::check() and
>      ObjectMonitor::check_slow() functions into a new function:
> 
>          ObjectMonitor::check_owner_and_throw_IMSE_if_not()

Bikeshed: sorry but that is far too long a name. check_owner() suffices 
and the comments for it state what it does in regard to throwing IMSE. 
We don't name methods in a way that includes what exceptions they may 
result in - that's what documentation is for.

src/hotspot/share/runtime/objectMonitor.cpp

! // function is not called with CHECK because we want the IMSE to be the
! // only exception that is thrown from the call site when false is 
returned.

I didn't understand that comment until I saw the actual call site code.

   if (!check_owner_and_throw_IMSE_if_not(THREAD)) {
      assert(HAS_PENDING_EXCEPTION, "expected a pending exception here.");
      return;
    }

This piece of code is repeated three times so seems to be screaming out 
to still be a macro. I'm inclined to make some simpler adjustments to 
the macro and its commentary:

// Checks that the current THREAD owns this monitor and causes
// an immediate return if it doesn't. We don't use the CHECK macro
// because we want the IMSE to be the only exception that is thrown from
// the call site when false is returned. Any other pending exception is
// ignored.
#defined CHECK_OWNER()          \
   do {                          \
     if (!check_owner(THREAD)) { \
        assert(HAS_PENDING_EXCEPTION, "expected a pending IMSE here."); \
        return;                  \
      }                          \
   } while (false)

then have the new implementation:

// Returns true if the specified thread owns the ObjectMonitor.
// Otherwise returns false and throws IllegalMonitorStateException
// (IMSE). If there is a pending exception and the specified thread
// is not the owner, that exception will be replaced by the IMSE.
bool ObjectMonitor::check_owner(Thread* THREAD) {
   if (_owner == THREAD) {
     return true;
   }
   if (THREAD->is_lock_owned((address)_owner)) {
     _owner = THREAD;  // convert from BasicLock addr to Thread addr
     _recursions = 0;
     return true;
   }
   THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
              "current thread is not owner", false);
}


The call sites then remain the same, other than fixing this comment:

1200   // Throw IMSX or IEX.

I don't know what IEX is meant to be :)

Thanks,
David

>      Update the CHECK_OWNER and check() call sites to use
>      check_owner_and_throw_IMSE_if_not(). Update the comments
>      to make the intended semantics clear.
> 
> Also included a new test for when Object.wait(), Object.notify()
> and Object.notifyAll() are called a thread that doesn't own the
> Java monitor.
> 
> Test with Mach5 Tier[1-3] on the usual Oracle platforms. Verified
> that the new test passes on all Oracle platforms. It's a Tier1
> test but its duration is < 1s on every platform.
> 
> Thanks, in advance, for any comments, questions or suggestions.
> 
> Dan
>