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

[Doerr, Martin]

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