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

[Denghui Dong]

Hi Andrew,
Thanks for the review.
Because there is no test case to verify the correctness, I added some additional code snippets (not included in the patch), 
diff:
--- a/src/share/vm/jfr/recorder/service/jfrRecorderThreadLoop.cpp       Thu Jan 30 00:21:06 2020 +0000
+++ b/src/share/vm/jfr/recorder/service/jfrRecorderThreadLoop.cpp       Thu Feb 13 13:56:51 2020 +0800
@@ -30,11 +30,25 @@
 #include "runtime/mutexLocker.hpp"
 #include "runtime/thread.inline.hpp"

+class CrashCallback : public os::CrashProtectionCallback {
+ public:
+  void call() {
+    char* c = NULL;
+    *c = 'c';
+  }
+};
+
 //
 // Entry point for "JFR Recorder Thread" message loop.
 // The recorder thread executes service requests collected from the message system.
 //
 void recorderthread_entry(JavaThread* thread, Thread* unused) {
+  {
+    os::ThreadCrashProtection crash_protection;
+    CrashCallback cb;
+    crash_protection.call(cb);
+  }
+
   assert(thread != NULL, "invariant");
   #define START (msgs & (MSGBIT(MSG_START)))
   #define SHUTDOWN (msgs & MSGBIT(MSG_SHUTDOWN))

If I use the JDK without this patch to run "java -XX:StartFlightRecording version", the process will be aborted,
here is an error sample:
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f76e32247c1, pid=51529, tid=0x00007f765ffd3700
#
# JRE version: OpenJDK Runtime Environment (8.0_212) (build 1.8.0_212-b00)
# Java VM: OpenJDK 64-Bit Server VM (25.212-b00 mixed mode linux-amd64 compressed oops)
# Problematic frame:
# V  [libjvm.so+0x6887c1]  CrashCallback::call()+0x1
#
# Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /ssd1/home/ddh/dev/jdk8u-jfr-incubator1/hs_err_pid51529.log
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
#
Aborted

And everything is fine when I use the JDK with this patch.
Do you think it's ok?

Thanks
Denghui Dong



------------------------------------------------------------------
From:Andrew Dinn <adinn at redhat.com>
Send Time:2020年2月13日(星期四) 01:14
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 04/02/2020 03:59, Denghui Dong wrote:
>   I'm so sorry for the late reply.
>   You are right, backport is a simpler way, so I make a webrev for the
> backport of JDK-8183925 for jdk8u-jfr-incubator.(Not sure should I
> create a new thread for this backport)
>   The webrev has some obvious differences with the original changeset
> because some codes had been introduced in JDK-8223147.
>   Please help review it.
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8183925/
> Jira: https://bugs.openjdk.java.net/browse/JDK-8183925
> Original
> changeset: http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/786437c6344b
This is all rather messy in that the patch seems to be starting from the
point where the jfr-incubator repo already contains both the old
(WatcherThreadCrashProtection) and most of the new
(ThreadCrashProtection) code i.e. this 'backport' is essentially just
removing the old code. I'm still not clear how or why the new code was
already present but it looks like the differences between this patch and
the upstream one only exist because someone has already done a rather
clumsy job of importing the new functionality.

Anyway, whatever the reason we are here, it looks to me as if this patch
means that the code will now rely on a correct copy of the upstream
ThreadCrashProtection and that all the redundant code for
WatcherThreadCrashProtection has been removed. So, from that point of
view the patch looks ok.

However, before we rush to commit this I'll note I don't see any mention
of testing. Can you explain what you have done to verify that this code
works as expected?

regards,


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