8231627: ThreadsListHandleInErrorHandlingTest.java fails in printing all threads

Reviewed-by: eosterlund, coleenp, pchilanomate, sspitsyn

src/hotspot/share/runtime/threadSMR.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1106,70 +1106,87 @@ void ThreadsSMRSupport::print_info_on(const Thread* thread, outputStream* st) {
 
 // Print Threads class SMR info.
 void ThreadsSMRSupport::print_info_on(outputStream* st) {
-  // Only grab the Threads_lock if we don't already own it and if we
+  bool needs_unlock = false;
-  // are not reporting an error.
+  if (Threads_lock->try_lock()) {
-  // Note: Not grabbing the Threads_lock during error reporting is
+    // We were able to grab the Threads_lock which makes things safe for
-  // dangerous because the data structures we want to print can be
+    // this call, but if we are error reporting, then a nested error
-  // freed concurrently. However, grabbing the Threads_lock during
+    // could happen with the Threads_lock held.
-  // error reporting can be equally dangerous since this thread might
+    needs_unlock = true;
-  // block during error reporting or a nested error could leave the
+  }
-  // Threads_lock held. The classic no win scenario.
-  //
-  MutexLocker ml((Threads_lock->owned_by_self() || VMError::is_error_reported()) ? NULL : Threads_lock);
 
-  st->print_cr("Threads class SMR info:");
+  ThreadsList* saved_threads_list = NULL;
-  st->print_cr("_java_thread_list=" INTPTR_FORMAT ", length=%u, "
+  {
-               "elements={", p2i(_java_thread_list),
+    ThreadsListHandle tlh;  // make the current ThreadsList safe for reporting
-               _java_thread_list->length());
+    saved_threads_list = tlh.list();  // save for later comparison
-  print_info_elements_on(st, _java_thread_list);
+
-  st->print_cr("}");
+    st->print_cr("Threads class SMR info:");
-  if (_to_delete_list != NULL) {
+    st->print_cr("_java_thread_list=" INTPTR_FORMAT ", length=%u, elements={",
-    st->print_cr("_to_delete_list=" INTPTR_FORMAT ", length=%u, "
+                 p2i(saved_threads_list), saved_threads_list->length());
-                 "elements={", p2i(_to_delete_list),
+    print_info_elements_on(st, saved_threads_list);
-                 _to_delete_list->length());
-    print_info_elements_on(st, _to_delete_list);
     st->print_cr("}");
-    for (ThreadsList *t_list = _to_delete_list->next_list();
+  }
-         t_list != NULL; t_list = t_list->next_list()) {
+
-      st->print("next-> " INTPTR_FORMAT ", length=%u, "
+  if (_to_delete_list != NULL) {
-                "elements={", p2i(t_list), t_list->length());
+    if (Threads_lock->owned_by_self()) {
-      print_info_elements_on(st, t_list);
+      // Only safe if we have the Threads_lock.
+      st->print_cr("_to_delete_list=" INTPTR_FORMAT ", length=%u, elements={",
+                   p2i(_to_delete_list), _to_delete_list->length());
+      print_info_elements_on(st, _to_delete_list);
       st->print_cr("}");
+      for (ThreadsList *t_list = _to_delete_list->next_list();
+           t_list != NULL; t_list = t_list->next_list()) {
+        st->print("next-> " INTPTR_FORMAT ", length=%u, elements={",
+                  p2i(t_list), t_list->length());
+        print_info_elements_on(st, t_list);
+        st->print_cr("}");
+      }
+    } else {
+      st->print_cr("_to_delete_list=" INTPTR_FORMAT, p2i(_to_delete_list));
+      st->print_cr("Skipping _to_delete_list fields and contents for safety.");
     }
   }
-  if (!EnableThreadSMRStatistics) {
+  if (EnableThreadSMRStatistics) {
-    return;
+    st->print_cr("_java_thread_list_alloc_cnt=" UINT64_FORMAT ", "
+                 "_java_thread_list_free_cnt=" UINT64_FORMAT ", "
+                 "_java_thread_list_max=%u, "
+                 "_nested_thread_list_max=%u",
+                 _java_thread_list_alloc_cnt,
+                 _java_thread_list_free_cnt,
+                 _java_thread_list_max,
+                 _nested_thread_list_max);
+    if (_tlh_cnt > 0) {
+      st->print_cr("_tlh_cnt=%u"
+                   ", _tlh_times=%u"
+                   ", avg_tlh_time=%0.2f"
+                   ", _tlh_time_max=%u",
+                   _tlh_cnt, _tlh_times,
+                   ((double) _tlh_times / _tlh_cnt),
+                   _tlh_time_max);
+    }
+    if (_deleted_thread_cnt > 0) {
+      st->print_cr("_deleted_thread_cnt=%u"
+                   ", _deleted_thread_times=%u"
+                   ", avg_deleted_thread_time=%0.2f"
+                   ", _deleted_thread_time_max=%u",
+                   _deleted_thread_cnt, _deleted_thread_times,
+                   ((double) _deleted_thread_times / _deleted_thread_cnt),
+                   _deleted_thread_time_max);
+    }
+    st->print_cr("_delete_lock_wait_cnt=%u, _delete_lock_wait_max=%u",
+                 _delete_lock_wait_cnt, _delete_lock_wait_max);
+    st->print_cr("_to_delete_list_cnt=%u, _to_delete_list_max=%u",
+                 _to_delete_list_cnt, _to_delete_list_max);
   }
-  st->print_cr("_java_thread_list_alloc_cnt=" UINT64_FORMAT ", "
+  if (needs_unlock) {
-               "_java_thread_list_free_cnt=" UINT64_FORMAT ", "
+    Threads_lock->unlock();
-               "_java_thread_list_max=%u, "
+  } else {
-               "_nested_thread_list_max=%u",
+    if (_java_thread_list != saved_threads_list) {
-               _java_thread_list_alloc_cnt,
+      st->print_cr("The _java_thread_list has changed from " INTPTR_FORMAT
-               _java_thread_list_free_cnt,
+                   " to " INTPTR_FORMAT
-               _java_thread_list_max,
+                   " so some of the above information may be stale.",
-               _nested_thread_list_max);
+                   p2i(saved_threads_list), p2i(_java_thread_list));
-  if (_tlh_cnt > 0) {
+    }
-    st->print_cr("_tlh_cnt=%u"
-                 ", _tlh_times=%u"
-                 ", avg_tlh_time=%0.2f"
-                 ", _tlh_time_max=%u",
-                 _tlh_cnt, _tlh_times,
-                 ((double) _tlh_times / _tlh_cnt),
-                 _tlh_time_max);
   }
-  if (_deleted_thread_cnt > 0) {
-    st->print_cr("_deleted_thread_cnt=%u"
-                 ", _deleted_thread_times=%u"
-                 ", avg_deleted_thread_time=%0.2f"
-                 ", _deleted_thread_time_max=%u",
-                 _deleted_thread_cnt, _deleted_thread_times,
-                 ((double) _deleted_thread_times / _deleted_thread_cnt),
-                 _deleted_thread_time_max);
-  }
-  st->print_cr("_delete_lock_wait_cnt=%u, _delete_lock_wait_max=%u",
-               _delete_lock_wait_cnt, _delete_lock_wait_max);
-  st->print_cr("_to_delete_list_cnt=%u, _to_delete_list_max=%u",
-               _to_delete_list_cnt, _to_delete_list_max);
 }
 
 // Print ThreadsList elements (4 per line).

test/hotspot/jtreg/runtime/ErrorHandling/NestedThreadsListHandleInErrorHandlingTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -93,8 +93,10 @@ public class NestedThreadsListHandleInErrorHandlingTest {
         Pattern.compile("Current thread .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=1, _nested_threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*.*"),
         // We should have a section of Threads class SMR info:
         Pattern.compile("Threads class SMR info:"),
-        // We should have one nested ThreadsListHandle:
+        // We should have had a double nested ThreadsListHandle since
-        Pattern.compile(".*, _nested_thread_list_max=1"),
+        // ThreadsSMRSupport::print_info_on() now protects itself with
+        // a ThreadsListHandle in addition to what the test creates:
+        Pattern.compile(".*, _nested_thread_list_max=2"),
         // The current thread (marked with '=>') in the threads list
         // should show a hazard ptr and a nested hazard ptr:
         Pattern.compile("=>.* JavaThread \"main\" .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=1, _nested_threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*.*"),

test/hotspot/jtreg/runtime/ErrorHandling/ThreadsListHandleInErrorHandlingTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -93,6 +93,10 @@ public class ThreadsListHandleInErrorHandlingTest {
         Pattern.compile("Current thread .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=0.*"),
         // We should have a section of Threads class SMR info:
         Pattern.compile("Threads class SMR info:"),
+        // We should have had a single nested ThreadsListHandle since
+        // ThreadsSMRSupport::print_info_on() now protects itself with
+        // a ThreadsListHandle:
+        Pattern.compile(".*, _nested_thread_list_max=1"),
         // The current thread (marked with '=>') in the threads list
         // should show a hazard ptr and no nested hazard ptrs:
         Pattern.compile("=>.* JavaThread \"main\" .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=0.*"),