RFR(T): 8229925: [s390] Exception check missing in interpreter

[David Holmes]

Hi Martin,

On 22/08/2019 10:19 pm, Doerr, Martin wrote:
> Hi David,
> 
> after spending more time for looking more deeply into build_method_counters I found a couple of locks which also contain a safepoint check. So making it a leaf call is not that simple.
> 
> I'll push webrev.00 if you're ok with that.

I'm fine with webrev.00, but you need a second reviewer.

Thanks,
David

> Thanks and best regards,
> Martin
> 
> 
>> -----Original Message-----
>> From: Doerr, Martin
>> Sent: Donnerstag, 22. August 2019 10:30
>> To: David Holmes <david.holmes at oracle.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: RE: RFR(T): 8229925: [s390] Exception check missing in interpreter
>>
>> Hi David,
>>
>>> I'm not following. The original issue is caused by not passing "true"
>>> for the check_exceptions parameter of call_VM. If you call call_VM_leaf
>>> then its effectively the same as the original problematic code in that
>>> there is no exception check.
>>
>> We just need to make sure an exception check follows each safepoint
>> (because an asnyc exception may get installed at it).
>>
>> Webrev.00 fixes the issue by checking for an exception after the safepoint
>> which is part of the ThreadInVMfromJava destructor (which is implemented
>> as part of JRT_ENTRY).
>>
>> My other idea fixes the issue by avoiding the safepoint at this point.
>> JRT_LEAF does not introduce any safepoint. So
>> InterpreterRuntime::build_method_counters gets executed without any
>> safepoint check and hence no asnyc exception can get installed within this
>> function. The Java thread will stop at the next safepoint outside of this
>> function and receive the async exception there in the scenario of this issue
>> (in this case the exception check in TemplateTable::branch which follows just
>> after the profiling code).
>>
>> I could just push webrev.00 which fixes the issue and is fine with me. But I
>> thought it may be interesting to discuss the alternative idea which simplifies
>> things a little more. Hoping this does not steal too much of your time.
>>
>> Thanks and best regards,
>> Martin
>>
>>
>>> -----Original Message-----
>>> From: David Holmes <david.holmes at oracle.com>
>>> Sent: Donnerstag, 22. August 2019 07:57
>>> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
>>> dev at openjdk.java.net
>>> Subject: Re: RFR(T): 8229925: [s390] Exception check missing in interpreter
>>>
>>> On 22/08/2019 12:46 am, Doerr, Martin wrote:
>>>> Hi David,
>>>>
>>>>> Update looks good.
>>>> Thanks.
>>>>
>>>>> The fact we may generate an OOME suggests straight away that we may
>>> hit
>>>>> a safepoint which violates the rules for JRT_LEAF.
>>>> No, just the pre-allocated Universe::_out_of_memory_error_metaspace
>>> or Universe::_out_of_memory_error_class_metaspace may get installed.
>>>
>>> Okay - I didn't deep dive into exactly where all those codepaths may
>>> lead. The point is JPRT_LEAF has a number of rules and its far from
>>> obvious to me that this code necessarily always obeys them.
>>>
>>>> JRT_LEAF contains a NoSafepointVerifier so we should notice a violation.
>>>>
>>>>> If it were a leaf then the exception check would be missing,
>>>>> reintroducing the test failure that highlighted the issue.
>>>> Note that the safepoint at which the async exception gets installed is part
>>> of the ThreadInVMfromJava destructor.
>>>
>>> I'm not following. The original issue is caused by not passing "true"
>>> for the check_exceptions parameter of call_VM. If you call call_VM_leaf
>>> then its effectively the same as the original problematic code in that
>>> there is no exception check.
>>>
>>> David
>>> -----
>>>
>>>> And InterpreterRuntime::build_method_counters uses
>>> CLEAR_PENDING_EXCEPTION before that.
>>>> If we use JRT_LEAF: no safepoint => no exception at this point.
>>>>> Thread* THREAD = thread;
>>>>>
>>>>> would be clearer (and consistent with VM_ENTRY macros)
>>>> Agreed.
>>>>
>>>> Best regards,
>>>> Martin
>>>>