[8u] [JFR] RFR: 8236160: Missing ThreadCrashProtection for JFR in signal handler

[Andrew Dinn]

On 19/12/2019 02:52, Denghui Dong wrote:
> Hi Andrew,
>   Thanks for the reply.
>   As I said in my last message, merging os::ThreadCrashProtection and
> os::WatcherThreadCrashProtection is a more elegant way,
> actually this work was done by
> JDK-8183925(Decouple crash protection from watcher thread), but it's not
> JFR-related.
>   And the first patch of JFR-backport use os::ThreadCrashProtection to
> protect jfr thread sampler crash, but miss check_crash_protection in the
> signal handler,
> so I made the changeset.
>   Do you think we should backport JDK-8183925 to jdk8u-jfr-incubator?

I'm still not at all clear what you /have/ backported.

Your patch inserts calls to

  os::ThreadCrashProtection::check_crash_protection(sig, t);

into the code that calls

  os::WatcherThreadCrashProtection::check_crash_protection()

If you have not already backported JDK-8183925 then how can those calls
be valid?

Have you just backported some of the patch? For example, have you added
os::ThreadCrashProtection to jdk8u-jfr-incubator without removing
WatcherThreadCrashProtection? If so then why woudld you not instead
simply backport all of JDK-8183925?

regards,


Andrew Dinn
-----------

>     ------------------------------------------------------------------
>     From:Andrew Dinn <adinn at redhat.com>
>     Send Time:2019年12月19日(星期四) 01:17
>     To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; jdk8u-dev
>     <jdk8u-dev at openjdk.java.net>
>     Subject:Re: [8u] [JFR] RFR: 8236160: Missing ThreadCrashProtection
>     for JFR in signal handler
> 
>     Hi Denghui,
> 
>     On 18/12/2019 06:37, Denghui Dong wrote:
>     > Hi all,
>     >   Please help review the following changeset:
>     > Jira: https://bugs.openjdk.java.net/browse/JDK-8236160
>     > Webrev: http://cr.openjdk.java.net/~ddong/8236160/webrev.00/hotspot.changeset
>     > Summary:
>     > JFR thread sampler uses ThreadCrashProtection to protect thread crash, so it's necessary to call
>     > ThreadCrashProtection::check_crash_protection in signal handler.
>     > And a more elegant way is to merge ThreadCrashProtection and WatcherThreadCrashProtection, but this changeset didn't do that.
>     I'm not clear why you are calling both
> 
>       os::WatcherThreadCrashProtection::check_crash_protection(sig, t);
> 
>     and
> 
>       os::ThreadCrashProtection::check_crash_protection(sig, t);
> 
>     Is it not possible simply to rely on os::ThreadCrashProtection alone as
>     happens updtream?
> 
>     I am assuming that you have already back-ported class
>     os::ThreadCrashProtection to the JFR incubator repo? Did you need to
>     change it's behaviour in any way in order to do that? Does that have
>     anything to do with why your are calling both routines?
> 
>     Also, how have you tested this patch?
> 
>     regards,
> 
> 
>     Andrew Dinn
>     -----------
>     Senior Principal Software Engineer
>     Red Hat UK Ltd
>     Registered in England and Wales under Company Registration No. 03798903
>     Directors: Michael Cunningham, Michael ("Mike") O'Neill
>