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

[Martin Doerr]

On Fri, 25 Sep 2020 12:07:40 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> 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?

Good point. We could use set_last_Java_frame + call_VM_leaf + reset_last_Java_frame, but that's a bit complicated. I'd
be ok with removing interpreter_frame_verify_monitor. If hs_err file dumping is good enough, we should get the desired
information, too.

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

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