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

[Reingruber, Richard]

Hi Martin,

looks good to me.

Cheers, Richard.

-----Original Message-----
From: Doerr, Martin <martin.doerr at sap.com> 
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: 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