8258479: Minor cleanups in VMError

Reviewed-by: lfoltan, coleenp

src/hotspot/os/posix/vmError_posix.cpp

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2020 SAP SE. 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
@@ -132,7 +133,7 @@ static void crash_handler(int sig, siginfo_t* info, void* ucVoid) {
   VMError::report_and_die(NULL, sig, pc, info, ucVoid);
 }
 
-void VMError::reset_signal_handlers() {
+void VMError::install_secondary_signal_handler() {
   for (int i = 0; i < NUM_SIGNALS; i++) {
     save_signal(i, SIGNALS[i]);
     os::signal(SIGNALS[i], CAST_FROM_FN_PTR(void *, crash_handler));

src/hotspot/os/windows/vmError_windows.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2020, 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
@@ -44,7 +44,7 @@ LONG WINAPI crash_handler(struct _EXCEPTION_POINTERS* exceptionInfo) {
   return EXCEPTION_CONTINUE_SEARCH;
 }
 
-void VMError::reset_signal_handlers() {
+void VMError::install_secondary_signal_handler() {
   SetUnhandledExceptionFilter(crash_handler);
 }
 

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp

@@ -603,7 +603,7 @@ HeapWord* ParallelScavengeHeap::block_start(const void* addr) const {
     assert(young_gen()->is_in(addr),
            "addr should be in allocated part of young gen");
     // called from os::print_location by find or VMError
-    if (Debugging || VMError::fatal_error_in_progress())  return NULL;
+    if (Debugging || VMError::is_error_reported())  return NULL;
     Unimplemented();
   } else if (old_gen()->is_in_reserved(addr)) {
     assert(old_gen()->is_in(addr),

src/hotspot/share/runtime/threadSMR.cpp

@@ -540,8 +540,7 @@ void SafeThreadsListPtr::verify_hazard_ptr_scanned() {
     return;
   }
 
-  if (VMError::is_error_reported() &&
-      VMError::get_first_error_tid() == os::current_thread_id()) {
+  if (VMError::is_error_reported_in_current_thread()) {
     // If there is an error reported by this thread it may use ThreadsList even
     // if it's unsafe.
     return;

src/hotspot/share/utilities/decoder.cpp

@@ -1,5 +1,6 @@
 /*
- * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020 SAP SE. 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
@@ -25,7 +26,6 @@
 #include "precompiled.hpp"
 #include "jvm.h"
 #include "memory/allocation.inline.hpp"
-#include "runtime/os.hpp"
 #include "utilities/decoder.hpp"
 #include "utilities/vmError.hpp"
 
@@ -84,30 +84,25 @@ Mutex* Decoder::shared_decoder_lock() {
 }
 
 bool Decoder::decode(address addr, char* buf, int buflen, int* offset, const char* modulepath, bool demangle) {
-  bool error_handling_thread = os::current_thread_id() == VMError::get_first_error_tid();
-  if (error_handling_thread) {
+  if (VMError::is_error_reported_in_current_thread()) {
     return get_error_handler_instance()->decode(addr, buf, buflen, offset, modulepath, demangle);
   } else {
     MutexLocker locker(shared_decoder_lock(), Mutex::_no_safepoint_check_flag);
     return get_shared_instance()->decode(addr, buf, buflen, offset, modulepath, demangle);
   }
-
 }
 
 bool Decoder::decode(address addr, char* buf, int buflen, int* offset, const void* base) {
-  bool error_handling_thread = os::current_thread_id() == VMError::get_first_error_tid();
-  if (error_handling_thread) {
+  if (VMError::is_error_reported_in_current_thread()) {
     return get_error_handler_instance()->decode(addr, buf, buflen, offset, base);
   } else {
     MutexLocker locker(shared_decoder_lock(), Mutex::_no_safepoint_check_flag);
     return get_shared_instance()->decode(addr, buf, buflen, offset, base);
   }
 }
 
-
 bool Decoder::demangle(const char* symbol, char* buf, int buflen) {
-  bool error_handling_thread = os::current_thread_id() == VMError::get_first_error_tid();
-  if (error_handling_thread) {
+  if (VMError::is_error_reported_in_current_thread()) {
     return get_error_handler_instance()->demangle(symbol, buf, buflen);
   } else {
     MutexLocker locker(shared_decoder_lock(), Mutex::_no_safepoint_check_flag);

src/hotspot/share/utilities/events.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2020, 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
@@ -127,7 +127,7 @@ template <class T> class EventLogBase : public EventLog {
   bool should_log() {
     // Don't bother adding new entries when we're crashing.  This also
     // avoids mutating the ring buffer when printing the log.
-    return !VMError::fatal_error_in_progress();
+    return !VMError::is_error_reported();
   }
 
   // Print the contents of the log

src/hotspot/share/utilities/vmError.cpp

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020 SAP SE. 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
@@ -63,10 +64,25 @@
 #include <signal.h>
 #endif // PRODUCT
 
-bool VMError::_error_reported = false;
-
-// call this when the VM is dying--it might loosen some asserts
-bool VMError::is_error_reported() { return _error_reported; }
+bool              VMError::coredump_status;
+char              VMError::coredump_message[O_BUFLEN];
+int               VMError::_current_step;
+const char*       VMError::_current_step_info;
+volatile jlong    VMError::_reporting_start_time = -1;
+volatile bool     VMError::_reporting_did_timeout = false;
+volatile jlong    VMError::_step_start_time = -1;
+volatile bool     VMError::_step_did_timeout = false;
+volatile intptr_t VMError::_first_error_tid = -1;
+int               VMError::_id;
+const char*       VMError::_message;
+char              VMError::_detail_msg[1024];
+Thread*           VMError::_thread;
+address           VMError::_pc;
+void*             VMError::_siginfo;
+void*             VMError::_context;
+const char*       VMError::_filename;
+int               VMError::_lineno;
+size_t            VMError::_size;
 
 // returns an address which is guaranteed to generate a SIGSEGV on read,
 // for test purposes, which is not NULL and contains bits in every word
@@ -80,7 +96,7 @@ void* VMError::get_segfault_address() {
 }
 
 // List of environment variables that should be reported in error log file.
-const char *env_list[] = {
+static const char* env_list[] = {
   // All platforms
   "JAVA_HOME", "JAVA_TOOL_OPTIONS", "_JAVA_OPTIONS", "CLASSPATH",
   "PATH", "USERNAME",
@@ -151,9 +167,6 @@ static void print_bug_submit_message(outputStream *out, Thread *thread) {
   out->print_raw_cr("#");
 }
 
-bool VMError::coredump_status;
-char VMError::coredump_message[O_BUFLEN];
-
 void VMError::record_coredump_status(const char* message, bool status) {
   coredump_status = status;
   strncpy(coredump_message, message, sizeof(coredump_message));
@@ -358,40 +371,15 @@ static void report_vm_version(outputStream* st, char* buf, int buflen) {
                );
 }
 
-// This is the main function to report a fatal error. Only one thread can
-// call this function, so we don't need to worry about MT-safety. But it's
-// possible that the error handler itself may crash or die on an internal
-// error, for example, when the stack/heap is badly damaged. We must be
-// able to handle recursive errors that happen inside error handler.
-//
-// Error reporting is done in several steps. If a crash or internal error
-// occurred when reporting an error, the nested signal/exception handler
-// can skip steps that are already (or partially) done. Error reporting will
-// continue from the next step. This allows us to retrieve and print
-// information that may be unsafe to get after a fatal error. If it happens,
-// you may find nested report_and_die() frames when you look at the stack
-// in a debugger.
-//
-// In general, a hang in error handler is much worse than a crash or internal
-// error, as it's harder to recover from a hang. Deadlock can happen if we
-// try to grab a lock that is already owned by current thread, or if the
-// owner is blocked forever (e.g. in os::infinite_sleep()). If possible, the
-// error handler and all the functions it called should avoid grabbing any
-// lock. An important thing to notice is that memory allocation needs a lock.
-//
-// We should avoid using large stack allocated buffers. Many errors happen
-// when stack space is already low. Making things even worse is that there
-// could be nested report_and_die() calls on stack (see above). Only one
-// thread can report error, so large buffers are statically allocated in data
-// segment.
-
-int          VMError::_current_step;
-const char*  VMError::_current_step_info;
+// Returns true if at least one thread reported a fatal error and fatal error handling is in process.
+bool VMError::is_error_reported() {
+  return _first_error_tid != -1;
+}
 
-volatile jlong VMError::_reporting_start_time = -1;
-volatile bool VMError::_reporting_did_timeout = false;
-volatile jlong VMError::_step_start_time = -1;
-volatile bool VMError::_step_did_timeout = false;
+// Returns true if the current thread reported a fatal error.
+bool VMError::is_error_reported_in_current_thread() {
+  return _first_error_tid == os::current_thread_id();
+}
 
 // Helper, return current timestamp for timeout handling.
 jlong VMError::get_current_timestamp() {
@@ -422,6 +410,32 @@ void VMError::clear_step_start_time() {
   return Atomic::store(&_step_start_time, (jlong)0);
 }
 
+// This is the main function to report a fatal error. Only one thread can
+// call this function, so we don't need to worry about MT-safety. But it's
+// possible that the error handler itself may crash or die on an internal
+// error, for example, when the stack/heap is badly damaged. We must be
+// able to handle recursive errors that happen inside error handler.
+//
+// Error reporting is done in several steps. If a crash or internal error
+// occurred when reporting an error, the nested signal/exception handler
+// can skip steps that are already (or partially) done. Error reporting will
+// continue from the next step. This allows us to retrieve and print
+// information that may be unsafe to get after a fatal error. If it happens,
+// you may find nested report_and_die() frames when you look at the stack
+// in a debugger.
+//
+// In general, a hang in error handler is much worse than a crash or internal
+// error, as it's harder to recover from a hang. Deadlock can happen if we
+// try to grab a lock that is already owned by current thread, or if the
+// owner is blocked forever (e.g. in os::infinite_sleep()). If possible, the
+// error handler and all the functions it called should avoid grabbing any
+// lock. An important thing to notice is that memory allocation needs a lock.
+//
+// We should avoid using large stack allocated buffers. Many errors happen
+// when stack space is already low. Making things even worse is that there
+// could be nested report_and_die() calls on stack (see above). Only one
+// thread can report error, so large buffers are statically allocated in data
+// segment.
 void VMError::report(outputStream* st, bool _verbose) {
 
 # define BEGIN if (_current_step == 0) { _current_step = __LINE__;
@@ -654,7 +668,6 @@ void VMError::report(outputStream* st, bool _verbose) {
        os::print_summary_info(st, buf, sizeof(buf));
      }
 
-
   STEP("printing date and time")
 
      if (_verbose) {
@@ -695,7 +708,6 @@ void VMError::report(outputStream* st, bool _verbose) {
         }
      }
 
-
   STEP("printing stack bounds")
 
      if (_verbose) {
@@ -1240,8 +1252,6 @@ void VMError::print_vm_info(outputStream* st) {
   st->print_cr("END.");
 }
 
-volatile intptr_t VMError::_first_error_tid = -1;
-
 /** Expand a pattern into a buffer starting at pos and open a file using constructed path */
 static int expand_and_open(const char* pattern, bool overwrite_existing, char* buf, size_t buflen, size_t pos) {
   int fd = -1;
@@ -1298,17 +1308,6 @@ static int prepare_log_file(const char* pattern, const char* default_pattern, bo
   return fd;
 }
 
-int         VMError::_id;
-const char* VMError::_message;
-char        VMError::_detail_msg[1024];
-Thread*     VMError::_thread;
-address     VMError::_pc;
-void*       VMError::_siginfo;
-void*       VMError::_context;
-const char* VMError::_filename;
-int         VMError::_lineno;
-size_t      VMError::_size;
-
 void VMError::report_and_die(Thread* thread, unsigned int sig, address pc, void* siginfo,
                              void* context, const char* detail_fmt, ...)
 {
@@ -1395,9 +1394,6 @@ void VMError::report_and_die(int id, const char* message, const char* detail_fmt
     _size = size;
     jio_vsnprintf(_detail_msg, sizeof(_detail_msg), detail_fmt, detail_args);
 
-    // first time
-    _error_reported = true;
-
     reporting_started();
     if (!TestUnresponsiveErrorHandler) {
       // Record reporting_start_time unless we're running the
@@ -1420,7 +1416,7 @@ void VMError::report_and_die(int id, const char* message, const char* detail_fmt
 
     // reset signal handlers or exception filter; make sure recursive crashes
     // are handled properly.
-    reset_signal_handlers();
+    install_secondary_signal_handler();
   } else {
 #if defined(_WINDOWS)
     // If UseOSErrorReporting we call this for each level of the call stack

src/hotspot/share/utilities/vmError.hpp

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020 SAP SE. 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
@@ -84,13 +85,9 @@ class VMError : public AllStatic {
   // Whether or not the last error reporting step did timeout.
   static volatile bool _step_did_timeout;
 
-  static bool _error_reported;
-
- public:
-
-  // set signal handlers on Solaris/Linux or the default exception filter
-  // on Windows, to handle recursive crashes.
-  static void reset_signal_handlers();
+  // Install secondary signal handler to handle secondary faults during error reporting
+  // (see VMError::crash_handler)
+  static void install_secondary_signal_handler();
 
   // handle -XX:+ShowMessageBoxOnError. buf is used to format the message string
   static void show_message_box(char* buf, int buflen);
@@ -171,18 +168,17 @@ class VMError : public AllStatic {
   // signal was not changed by error reporter
   static address get_resetted_sighandler(int sig);
 
-  // check to see if fatal error reporting is in progress
-  static bool fatal_error_in_progress() { return _first_error_tid != -1; }
-
-  static intptr_t get_first_error_tid() { return _first_error_tid; }
-
   // Called by the WatcherThread to check if error reporting has timed-out.
   //  Returns true if error reporting has not completed within the ErrorLogTimeout limit.
   static bool check_timeout();
 
-  // Support for avoiding multiple asserts
+  // Returns true if at least one thread reported a fatal error and
+  //  fatal error handling is in process.
   static bool is_error_reported();
 
+  // Returns true if the current thread reported a fatal error.
+  static bool is_error_reported_in_current_thread();
+
   DEBUG_ONLY(static void controlled_crash(int how);)
 
   // returns an address which is guaranteed to generate a SIGSEGV on read,