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

[David Holmes]

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
>