RFR: 8253540: InterpreterRuntime::monitorexit should be a JRT_LEAF function [v2]

[Daniel D. Daugherty]

Looks like this reply from David didn't end up getting added to the
PR by the SkaraBot... It will be interesting to see if my reply to
David's reply gets picked up by the SkaraBot... <evil laugh here>


Both Coleen and I crawled through the current set of code paths that
get us into InterpreterRuntime::monitorexit() and we have (mostly)
convinced ourselves that all code paths are covered by ownership
checks and the throwing of IllegalMonitorStateException. However, to
guard against future code breakage (or errors in our analysis) we
decided to add _something_ to catch errors here. We talked about
assert()s, guarantee()s and fatal() calls. Coleen settled on the
fatal() call guarded by CheckJNICalls. Since we believe that only
JNI code can screw this up (modulo new bugs in the interpreter or
compiler code generation), this seemed like the right way to go..

Dan


On 9/25/20 7:21 AM, David Holmes wrote:
> Hi Martin, Coleen,
>
> Well this is a little embarrasing so I need to clarify this :) The 
> "email" Martin received was presumably a direct notification from 
> github. I added a comment and then immediately realized my mistake and 
> deleted it before the skara bots sent out the email to the mailing 
> list. But the github notification was sent immediately. :(
>
> Please ignore the comment as I realised this was handled at the 
> previous level as Martin describes.
>
> Aside: I'm not at all sure it is even possible to hit the bit with the 
> CheckJNICalls given we must have found a valid BasicObjectLock to get 
> that far. But trying to figure out all the possible paths was too 
> complex - the check is a good safety net.
>
> Cheers,
> David
>
> On 25/09/2020 7:19 pm, Martin Doerr wrote:
>> On Thu, 24 Sep 2020 14:24:43 GMT, Coleen Phillimore 
>> <coleenp at openjdk.org> wrote:
>>
>>>> Please note that complete_monitor_unlocking_C is only called by C2. 
>>>> Other JIT compilers have other functions.
>>>> SharedRuntime::monitor_exit_helper is used by all ones. I had 
>>>> another question: Shouldn't the call sites in the
>>>> interpreter use call_VM_leaf instead of call_VM?
>>>
>>> @TheRealMDoerr you're right, it should be call_VM_leaf so that 
>>> popframe and force early return are not executed.
>>> Setting up last_Java_sp is benign but unnecessary.  I should have 
>>> changed the code in
>>> SharedRuntime::monitor_exit_helper instead.  Thanks for looking at 
>>> this and your help!
>>
>> Hi David,
>>
>> the IllegalMonitorStateException should get thrown by the code 
>> generated by TemplateTable::monitorexit() without
>> reaching InterpreterRuntime::monitorexit in this scenario. But we 
>> should review this carefully and run all such JCK
>> tests and whatever we have before pushing this change.
>>
>> Coleen’s check for already unlocked objects is only for JNI and is 
>> not mandatory AFAICS:
>> “Native code must not use MonitorExit to exit a monitor entered 
>> through a synchronized method or a monitorenter Java
>> virtual machine instruction.” 
>> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#MonitorExit
>> So Coleen has chosen to implement the undefined behavior such that we 
>> gracefully ignore it or report fatal with
>> CheckJNICalls.
>>
>> Best regards,
>> Martin
>>
>>
>> From: David Holmes <notifications at github.com>
>> Sent: Freitag, 25. September 2020 07:40
>> To: openjdk/jdk <jdk at noreply.github.com>
>> Cc: Doerr, Martin <martin.doerr at sap.com>; Mention 
>> <mention at noreply.github.com>
>> Subject: Re: [openjdk/jdk] 8253540: InterpreterRuntime::monitorexit 
>> should be a JRT_LEAF function (#320)
>>
>>
>> Hi Coleen,
>> I believe that failing to throw IllegalMonitorStateException now 
>> violates the JVMS. Even if the VM enforces balanced
>> locking it should still throw IMSE here. Consider a JCK-style test 
>> that just performs a monitorexit with no
>> monitorenter, expecting to get IMSE - such a test exists.
>>
>> Cheers,
>> David
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on 
>> GitHub<https://github.com/openjdk/jdk/pull/320#issuecomment-698731101>, 
>> or
>> unsubscribe<https://github.com/notifications/unsubscribe-auth/AKR64KHMNL5LMCHFPBFCNADSHQUKLANCNFSM4RXDWL3A>. 
>>
>>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/320
>>