8271588: JFR Recorder Thread crashed with SIGSEGV in write_klass

Markus Grönlund · Markus Grönlund · commit 90f85ff70fe6 · 2021-08-05T09:13:26.000Z
Reviewed-by: egahlin
diff --git a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp
@@ -455,7 +455,7 @@ size_t JfrCheckpointManager::flush_type_set() {
       elements = ::flush_type_set(thread);
     }
   }
-  if (_new_checkpoint.is_signaled()) {
+  if (_new_checkpoint.is_signaled_with_reset()) {
     WriteOperation wo(_chunkwriter);
     MutexedWriteOperation mwo(wo);
     _thread_local_mspace->iterate(mwo); // current epoch list
diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp
@@ -1103,6 +1103,10 @@ void JfrTypeSet::clear() {
 }
 
 size_t JfrTypeSet::on_unloading_classes(JfrCheckpointWriter* writer) {
+  // JfrTraceIdEpoch::has_changed_tag_state_no_reset() is a load-acquire we issue to see side-effects (i.e. tags).
+  // The JfrRecorderThread does this as part of normal processing, but with concurrent class unloading, which can
+  // happen in arbitrary threads, we invoke it explicitly.
+  JfrTraceIdEpoch::has_changed_tag_state_no_reset();
   if (JfrRecorder::is_recording()) {
     return serialize(writer, NULL, true, false);
   }
diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp
@@ -108,6 +108,10 @@ class JfrTraceIdEpoch : AllStatic {
   }
 
   static bool has_changed_tag_state() {
+    return _tag_state.is_signaled_with_reset();
+  }
+
+  static bool has_changed_tag_state_no_reset() {
     return _tag_state.is_signaled();
   }
 
diff --git a/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp b/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp
@@ -44,7 +44,7 @@ typedef JfrStringPool::BufferPtr BufferPtr;
 static JfrSignal _new_string;
 
 bool JfrStringPool::is_modified() {
-  return _new_string.is_signaled();
+  return _new_string.is_signaled_with_reset();
 }
 
 static JfrStringPool* _instance = NULL;
diff --git a/src/hotspot/share/jfr/utilities/jfrSignal.hpp b/src/hotspot/share/jfr/utilities/jfrSignal.hpp
@@ -34,14 +34,16 @@ class JfrSignal {
   JfrSignal() : _signaled(false) {}
 
   void signal() const {
-    if (!Atomic::load_acquire(&_signaled)) {
-      Atomic::release_store(&_signaled, true);
-    }
+    Atomic::release_store(&_signaled, true);
   }
 
   bool is_signaled() const {
-    if (Atomic::load_acquire(&_signaled)) {
-      Atomic::release_store(&_signaled, false); // auto-reset
+    return Atomic::load_acquire(&_signaled);
+  }
+
+  bool is_signaled_with_reset() const {
+    if (is_signaled()) {
+      Atomic::release_store(&_signaled, false);
       return true;
     }
     return false;

Original file line number	Diff line number	Diff line change
`@@ -455,7 +455,7 @@ size_t JfrCheckpointManager::flush_type_set() {`
`455`	`455`	`elements = ::flush_type_set(thread);`
`456`	`456`	`}`
`457`	`457`	`}`
`458`		`- if (_new_checkpoint.is_signaled()) {`
	`458`	`+ if (_new_checkpoint.is_signaled_with_reset()) {`
`459`	`459`	`WriteOperation wo(_chunkwriter);`
`460`	`460`	`MutexedWriteOperation mwo(wo);`
`461`	`461`	`_thread_local_mspace->iterate(mwo); // current epoch list`
Original file line number	Diff line number	Diff line change
`@@ -1103,6 +1103,10 @@ void JfrTypeSet::clear() {`
`1103`	`1103`	`}`
`1104`	`1104`
`1105`	`1105`	`size_t JfrTypeSet::on_unloading_classes(JfrCheckpointWriter* writer) {`
	`1106`	`+ // JfrTraceIdEpoch::has_changed_tag_state_no_reset() is a load-acquire we issue to see side-effects (i.e. tags).`
	`1107`	`+ // The JfrRecorderThread does this as part of normal processing, but with concurrent class unloading, which can`
	`1108`	`+ // happen in arbitrary threads, we invoke it explicitly.`
	`1109`	`+ JfrTraceIdEpoch::has_changed_tag_state_no_reset();`
`1106`	`1110`	`if (JfrRecorder::is_recording()) {`
`1107`	`1111`	`return serialize(writer, NULL, true, false);`
`1108`	`1112`	`}`
Original file line number	Diff line number	Diff line change
`@@ -108,6 +108,10 @@ class JfrTraceIdEpoch : AllStatic {`
`108`	`108`	`}`
`109`	`109`
`110`	`110`	`static bool has_changed_tag_state() {`
	`111`	`+ return _tag_state.is_signaled_with_reset();`
	`112`	`+ }`
	`113`	`+`
	`114`	`+ static bool has_changed_tag_state_no_reset() {`
`111`	`115`	`return _tag_state.is_signaled();`
`112`	`116`	`}`
`113`	`117`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ typedef JfrStringPool::BufferPtr BufferPtr;`
`44`	`44`	`static JfrSignal _new_string;`
`45`	`45`
`46`	`46`	`bool JfrStringPool::is_modified() {`
`47`		`- return _new_string.is_signaled();`
	`47`	`+ return _new_string.is_signaled_with_reset();`
`48`	`48`	`}`
`49`	`49`
`50`	`50`	`static JfrStringPool* _instance = NULL;`