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

[Martin Doerr]

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