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

[Coleen Phillimore]

On 9/25/20 11:18 AM, Daniel D. Daugherty wrote:
> 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>
>

This thread isn't going through the SkaraBot.  I can't tell you exactly why.

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

Yes, thank you very much for looking through this and our discussions 
offline.  It would have been another 100 email messages.  Thank you for 
the summary.

Coleen

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