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

[David Holmes]

Hi Martin,

On 21/08/2019 10:54 pm, Doerr, Martin wrote:
> Hi David,
> 
> thank you very much for reviewing and for checking the other platforms.
> I agree. PPC64 should also perform the exception check. Even though we have never seen this failure on PPC64.
> I've updated the issue and the webrev.00 in place.

Update looks good.

> After looking at InterpreterRuntime::build_method_counters, I wonder why it has been designed as JRT_ENTRY and not as JRT_LEAF. I couldn't find anything which requires the transition ThreadInVMfromJava and the safepoint check when leaving.

The fact we may generate an OOME suggests straight away that we may hit 
a safepoint which violates the rules for JRT_LEAF.

> Changing it to a leaf call would also be a valid fix (PPC64 example [1] below). What do you think?

If it were a leaf then the exception check would be missing, 
reintroducing the test failure that highlighted the issue.

> Best regards,
> Martin
> 
> 
> 
> [1] leaf call
> diff -r 40006c0ada91 src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp
> --- a/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Wed Aug 21 00:08:35 2019 +0200
> +++ b/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp        Wed Aug 21 14:36:57 2019 +0200
> @@ -2264,8 +2264,7 @@
>     ld(Rcounters, in_bytes(Method::method_counters_offset()), method);
>     cmpdi(CCR0, Rcounters, 0);
>     bne(CCR0, has_counters);
> -  call_VM(noreg, CAST_FROM_FN_PTR(address,
> -                                  InterpreterRuntime::build_method_counters), method, false);
> +  call_VM_leaf(CAST_FROM_FN_PTR(address, InterpreterRuntime::build_method_counters), R16_thread, method);
>     ld(Rcounters, in_bytes(Method::method_counters_offset()), method);
>     cmpdi(CCR0, Rcounters, 0);
>     beq(CCR0, skip); // No MethodCounters, OutOfMemory.
> diff -r 40006c0ada91 src/hotspot/share/interpreter/interpreterRuntime.cpp
> --- a/src/hotspot/share/interpreter/interpreterRuntime.cpp      Wed Aug 21 00:08:35 2019 +0200
> +++ b/src/hotspot/share/interpreter/interpreterRuntime.cpp      Wed Aug 21 14:36:57 2019 +0200
> @@ -1190,8 +1190,9 @@
>     last_frame.set_mdp(new_mdp);
>   JRT_END
> 
> -JRT_ENTRY(MethodCounters*, InterpreterRuntime::build_method_counters(JavaThread* thread, Method* m))
> +JRT_LEAF(MethodCounters*, InterpreterRuntime::build_method_counters(JavaThread* thread, Method* m))
>     MethodCounters* mcs = Method::build_method_counters(m, thread);
> +  TRAPS = thread;

That's a little obscure.

Thread* THREAD = thread;

would be clearer (and consistent with VM_ENTRY macros)

Cheers,
David

>     if (HAS_PENDING_EXCEPTION) {
>       assert((PENDING_EXCEPTION->is_a(SystemDictionary::OutOfMemoryError_klass())), "we expect only an OOM error here");
>       CLEAR_PENDING_EXCEPTION;
> 
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Mittwoch, 21. August 2019 09:31
>> 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
>>
>> Hi Martin,
>>
>> On 21/08/2019 1:32 am, Doerr, Martin wrote:
>>> Hi,
>>>
>>> we have sporadically seen "assert(!(((ThreadShadow*)__the_thread__)-
>>> has_pending_exception())) failed: Should not have any exceptions
>> pending" on s390 while running jtreg test
>> "vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001".
>>> The VM asserts that no exception is pending after a backedge counter
>> overflow. The unexpected pending exception found in
>> frequency_counter_overflow_inner is an instance of java.lang.ThreadDeath
>> which got installed asynchronously by the test via JVMTI StopThread.
>>> The VM call to InterpreterRuntime::build_method_counters misses an
>> exception check in the s390 implementation. This is necessary for
>> asynchronous exceptions which get installed by this test.
>>>
>>> The explanation may not be trivial, but the proposed fix is:
>>>
>> http://cr.openjdk.java.net/~mdoerr/8229925_s390_exception_check/webre
>> v.00/
>>
>> Seems quite reasonable given the explanation, and comparing similar code
>> to other platforms.
>>
>> PPC64 would seem to have the same bug:
>>
>>     call_VM(noreg, CAST_FROM_FN_PTR(address,
>>
>> InterpreterRuntime::build_method_counters), method, false);
>>
>> Cheers,
>> David
>> -----
>>
>>> Best regards,
>>> Martin
>>>