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

[David Holmes]

On 26/09/2020 4:45 am, Coleen Phillimore wrote:
> 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.

"It looks at the In-Reply-To header (from the mbox-formatted archive) to 
determine which mails are part of a PR-initiated conversation."

This one started from a git notification email.

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

Yes thanks for the additional info. As I said "the check is a good 
safety net".

Cheers,
David
-----

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