RFR: 8241234: Unify monitor enter/exit runtime entries

[David Holmes]

On 31/03/2020 1:41 am, coleen.phillimore at oracle.com wrote:
> On 3/30/20 11:20 AM, Daniel D. Daugherty wrote:
>> On 3/30/20 10:27 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 3/30/20 10:20 AM, Daniel D. Daugherty wrote:
>>>> On 3/30/20 10:15 AM, Yudi Zheng wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Thanks for the review! I have uploaded a new version with your 
>>>>> comments addressed:
>>>>> http://cr.openjdk.java.net/~yzheng/8241234/webrev.04/
>>>>>
>>>>>> src/hotspot/share/runtime/sharedRuntime.hpp
>>>>>>      Please don't forget to update the copyright year before you 
>>>>>> push.
>>>>> Fixed.
>>>>>
>>>>>> src/hotspot/share/runtime/sharedRuntime.cpp
>>>>>>      L2104:   ObjectSynchronizer::exit(obj, lock, THREAD);
>>>>>>          The use of 'THREAD' here and 'TRAPS' in the function itself
>>>>>>          standout more now, but that's something for me to cleanup.
>>>>> Also, I noticed that C2 was using CHECK
>>>>>>     ObjectSynchronizer::enter(h_obj, lock, CHECK);
>>>>> While C1 and JVMCI were using THREAD:
>>>>>>     ObjectSynchronizer::enter(h_obj, lock->lock(), THREAD);
>>>>> I have no idea when to use what, and hope unifying to the C2 
>>>>> entries would help.
>>>>> Let me know if there is something I should address in this patch. 
>>>>> Otherwise, I would
>>>>> rather leave it to the expert, i.e., you ;)
>>>>
>>>> Yes, please leave it for me to clean up.
>>>
>>> It appears that both of these places (monitor_enter_helper and 
>>> monitor_exit_helper) should be using CATCH.
>>
>> Here's CATCH:
>>
>> src/hotspot/share/utilities/exceptions.hpp:
>>
>> // The CATCH macro checks that no exception has been thrown by a 
>> function; it is used at
>> // call sites about which is statically known that the callee cannot 
>> throw an exception
>> // even though it is declared with TRAPS.
>>
>> #define CATCH                              \
>>   THREAD); if (HAS_PENDING_EXCEPTION) {    \
>>     oop ex = PENDING_EXCEPTION;            \
>>     CLEAR_PENDING_EXCEPTION;               \
>>     ex->print();                           \
>>     ShouldNotReachHere();                  \
>>   } (void)(0
>>
>>
>> I don't think you want either monitor_enter_helper() or
>> monitor_exit_helper() crashing the VM via a ShouldNotReachHere()
>> if someone happens to throw an exception at a thread during
>> a 'monitorenter' or 'monitorexit' bytecode.
> 
> Of course, it's more complicated.  I was looking at this:
> 
>     ObjectSynchronizer::enter(h_obj, lock, CHECK);
>     assert(!HAS_PENDING_EXCEPTION, "Should have no exception here");
>     JRT_BLOCK_END
> 
> Which essentially wants to, but fails to, assert that there's no 
> exceptions possible (it is enclosed with JRT_BLOCK_NO_ASYNC).
> 
> I can't see whether ObjectSynchronizer::enter() has code to exclude 
> async exceptions from being installed (possibly biased locking revoking 
> at a handshake?).
> 
> And this:
> 
> +void SharedRuntime::monitor_exit_helper(oopDesc* obj, BasicLock* lock, 
> JavaThread* thread) {
> + assert(JavaThread::current() == thread, "invariant");
>       // Exit must be non-blocking, and therefore no exceptions can be 
> thrown.
>       EXCEPTION_MARK;
>       ObjectSynchronizer::exit(obj, lock, THREAD);
> - }
> +}
> 
> Which has ExceptionMark whose destructor has:
> 
> fatal("ExceptionMark destructor expects no pending exceptions");
> 
> Which appears to be enabled in production.
> 
> So either way, you want the functionality of CHECK, just not have it 
> assert in production (which was a surprise to me).
> 
> Actually, I agree with you that you want to clean this up.

+1 for Dan to clean it up :)

Conceptually monitor entry should not generate any exceptions, but 
detection of async exceptions needs to be done very carefully.

Monitor exit can trigger IllegalMonitorStateException but the devil is 
in the detail as this feeds into the "balanced locking" rules. So 
whether or not it is possible depends on exactly where this code fits in.

David

> Coleen
>>
>> What am I missing here?
>>
>> Dan
>>
>>
>>>
>>> Coleen
>>>>
>>>>
>>>>>> src/hotspot/share/c1/c1_Runtime1.cpp
>>>>>>      old L718:   assert(thread == JavaThread::current(), "threads 
>>>>>> must correspond");
>>>>>>          Removed in favor of the assert in 
>>>>>> SharedRuntime::monitor_enter_helper().
>>>>>>          Okay that makes sense.
>>>>>>
>>>>>>      old L721:   EXCEPTION_MARK;
>>>>>>          Removed in favor of the same in 
>>>>>> SharedRuntime::monitor_enter_helper().
>>>>>>          Okay that makes sense.
>>>>>>
>>>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>>>      old L403:   assert(thread == JavaThread::current(), "threads 
>>>>>> must correspond");
>>>>>>      old L406:   EXCEPTION_MARK;
>>>>>>          Same as for c1_Runtime1.cpp
>>>>> I assume I don’t need to do anything regarding the comments above.
>>>>
>>>> Correct. Just observations on the old code.
>>>>
>>>>
>>>>>>      L390:     TRACE_jvmci_3("%s: entered locking slow case with 
>>>>>> obj="...
>>>>>>      L394:   TRACE_jvmci_3("%s: exiting locking slow with obj="
>>>>>>      L417:     TRACE_jvmci_3("%s: exited locking slow case with obj="
>>>>>>          But this is no longer the "slow" case so I'm a bit confused.
>>>>>>
>>>>>>          Update: I see there's a comment about the tracing being 
>>>>>> removed.
>>>>>>          I have no opinion on that since it is JVM/CI code, but 
>>>>>> the word
>>>>>>          "slow" needs to be adjusted if you keep it.
>>>>> I removed all the tracing code.
>>>>
>>>> Thanks for cleaning that up.
>>>>
>>>> Dan
>>>>
>>>>>
>>>>> Many thanks,
>>>>> Yudi
>>>>
>>>
>>
>