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

[Coleen Phillimore]

On Fri, 25 Sep 2020 09:16:32 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> @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>.

What Martin said.  There are too many emails ...

Further note, I implemented calling monitorexit with call_VM_leaf.  By doing so, I lose the code that sets up the
_last_Java_fp so that this check can't be made before and after ObjectSynchronizer::exit anymore:

#ifdef ASSERT
  thread->last_frame().interpreter_frame_verify_monitor(elem);
#endif

I don't know if there's an objection to removing it.  Maybe back in the day the vm was so buggy that ObjectSynchronizer
broke the monitorstack so someone added this?

The advantage to making the calls use call_VM_leaf is that call_VM_base also checks for popframe and earlyreturn from
the debugger.  I visually checked that these call paths don't inadvertently call_VM and install an asynchronous
exception (the bug I was chasing last year), but not having this code path saves visual or mechanical inspection and
would make the code more robust.  What are people's preferences?

-------------

PR: https://git.openjdk.java.net/jdk/pull/320