RFR(XS): 8232005: [s390] More exception checks missing in interpreter

[Lindenmaier, Goetz]

Hi Martin, 

Checking for additional exception looks good.
I see you changed the call to MacroAssember in templateInterpreterGenerator_ppc.cpp
to a call to interpreterMacroAssembler. Also you removed a "true", probably 
to clean up the code. Fine too.
I didn't check for more sites where exceptions need to be checked.

Thanks,
  Goetz.

> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net>
> On Behalf Of Doerr, Martin
> Sent: Mittwoch, 16. Oktober 2019 11:45
> To: Reingruber, Richard <richard.reingruber at sap.com>; hotspot-runtime-
> dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Langer,
> Christoph <christoph.langer at sap.com>
> Subject: [CAUTION] RE: RFR(XS): 8232005: [s390] More exception checks
> missing in interpreter
> 
> Hi Richard,
> 
> thanks a lot. I have come to the same conclusions.
> 
> Here's the new webrev:
> http://cr.openjdk.java.net/~mdoerr/8232005_s390_check_exceptions/webrev.
> 01/
> 
> Best regards,
> Martin
> 
> 
> > -----Original Message-----
> > From: Reingruber, Richard <richard.reingruber at sap.com>
> > Sent: Mittwoch, 16. Oktober 2019 10:05
> > To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> > dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Langer,
> > Christoph <christoph.langer at sap.com>
> > Subject: RE: RFR(XS): 8232005: [s390] More exception checks missing in
> > interpreter
> >
> > Hi Martin
> >
> >   > Regarding the s390 code, I think the additional places you mentioned are
> > no bugs.
> >   > Monitorenter/exit use JRT_ENTRY_NO_ASYNC, so no async exception will
> > get installed.
> >
> > You're right. I think InterpreterRuntime::monitorexit can return an
> > IllegalMonitorStateException,
> > though, if the object is already unlocked. Would be good if you could add the
> > exception check there.
> >
> > Cheers, Richard.
> >
> > -----Original Message-----
> > From: Doerr, Martin <martin.doerr at sap.com>
> > Sent: Montag, 14. Oktober 2019 19:15
> > To: Reingruber, Richard <richard.reingruber at sap.com>; hotspot-runtime-
> > dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Langer,
> > Christoph <christoph.langer at sap.com>
> > Subject: RE: RFR(XS): 8232005: [s390] More exception checks missing in
> > interpreter
> >
> > Hi Richard,
> >
> > thank you for reviewing and for checking other places, too.
> >
> > Regarding the s390 code, I think the additional places you mentioned are no
> > bugs.
> > Monitorenter/exit use JRT_ENTRY_NO_ASYNC, so no async exception will
> > get installed.
> > I guess the s390 code was designed this way by intention? No idea why x86
> > has exception checks, here.
> >
> > It would also be safe to omit the exception check at places, in which we
> > already have a pending exception which is handled by specific code later on.
> > Async exceptions will get postponed in this case. But this doesn't seem to be
> > the case for the places you mentioned.
> >
> > Regarding the PPC64 code, I think we should fix the places you found. They
> > seem to be real bugs AFAICS.
> >
> > Best regards,
> > Martin
> >
> >
> > > -----Original Message-----
> > > From: Reingruber, Richard <richard.reingruber at sap.com>
> > > Sent: Freitag, 11. Oktober 2019 11:36
> > > To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> > > dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Langer,
> > > Christoph <christoph.langer at sap.com>
> > > Subject: RE: RFR(XS): 8232005: [s390] More exception checks missing in
> > > interpreter
> > >
> > > Hi Martin,
> > >
> > > Thanks for analyzing and fixing those.
> > >
> > > Have you looked at the following:
> > >
> > > cpu/s390/interp_masm_s390.cpp:970:    call_VM(noreg,
> > > CAST_FROM_FN_PTR(address, InterpreterRuntime::monitorenter),
> > > cpu/s390/interp_masm_s390.cpp-971-            monitor,
> > > /*check_for_exceptions=*/false);
> > > cpu/s390/interp_masm_s390.cpp:1150:  call_VM(noreg,
> > > CAST_FROM_FN_PTR(address, InterpreterRuntime::monitorexit),
> > > cpu/s390/interp_masm_s390.cpp-1151-          monitor,
> > > /*check_for_exceptions=*/false);
> > > cpu/ppc/templateInterpreterGenerator_ppc.cpp:595:    __
> > > call_VM(Rexception, CAST_FROM_FN_PTR(address,
> > > InterpreterRuntime::create_klass_exception), false);
> > > cpu/ppc/templateInterpreterGenerator_ppc.cpp:598:    __
> > > call_VM(Rexception, CAST_FROM_FN_PTR(address,
> > > InterpreterRuntime::create_exception), false);
> > > cpu/ppc/templateInterpreterGenerator_ppc.cpp:2108:    __
> > > MacroAssembler::call_VM(R4_ARG2, CAST_FROM_FN_PTR(address,
> > > InterpreterRuntime::member_name_arg_or_null), R4_ARG2,
> > R19_method,
> > > R14_bcp, false);
> > >
> > > x86 does the exception check there. I think the check is needed, because
> > > async. exceptions can get installed there and if we don't check, they could
> > be
> > > dropped.
> > >
> > > Thanks, Richard.
> > > (Not yet Reviewer)
> > >
> > > -----Original Message-----
> > > From: hotspot-runtime-dev <hotspot-runtime-dev-
> > > bounces at openjdk.java.net> On Behalf Of Doerr, Martin
> > > Sent: Dienstag, 8. Oktober 2019 16:42
> > > To: hotspot-runtime-dev at openjdk.java.net; Schmidt, Lutz
> > > <lutz.schmidt at sap.com>; Langer, Christoph <christoph.langer at sap.com>
> > > Subject: [CAUTION] RFR(XS): 8232005: [s390] More exception checks
> > missing
> > > in interpreter
> > >
> > > Hi,
> > >
> > > the template interpreter on s390 misses more exception checks when
> > calling
> > > VM functions which may install an async exception (like
> > > java.lang.ThreadDeath) at a safepoint.
> > >
> > > Bug:
> > > https://bugs.openjdk.java.net/browse/JDK-8232005
> > >
> > > These are the two places I found:
> > >
> > http://cr.openjdk.java.net/~mdoerr/8232005_s390_check_exceptions/webr
> > > ev.00/
> > >
> > > Please review.
> > >
> > > Best regards,
> > > Martin