8302189: Mark assertion failures noreturn

8302799: Refactor Debugging variable usage for noreturn crash reporting

Reviewed-by: dholmes, coleenp

make/hotspot/lib/CompileJvm.gmk

@@ -100,7 +100,7 @@ endif
 
 DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value
 
-DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146 4127
+DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146 4127 4722
 
 ################################################################################
 # Platform specific setup

src/hotspot/cpu/x86/macroAssembler_x86.cpp

@@ -42,7 +42,6 @@
 #include "oops/klass.inline.hpp"
 #include "prims/methodHandles.hpp"
 #include "runtime/continuation.hpp"
-#include "runtime/flags/flagSetting.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 #include "runtime/javaThread.hpp"
 #include "runtime/jniHandles.hpp"
@@ -403,7 +402,7 @@ void MacroAssembler::debug32(int rdi, int rsi, int rbp, int rsp, int rbx, int rd
 
 void MacroAssembler::print_state32(int rdi, int rsi, int rbp, int rsp, int rbx, int rdx, int rcx, int rax, int eip) {
   ttyLocker ttyl;
-  FlagSetting fs(Debugging, true);
+  DebuggingContext debugging{};
   tty->print_cr("eip = 0x%08x", eip);
 #ifndef PRODUCT
   if ((WizardMode || Verbose) && PrintMiscellaneous) {
@@ -832,7 +831,7 @@ void MacroAssembler::debug64(char* msg, int64_t pc, int64_t regs[]) {
 
 void MacroAssembler::print_state64(int64_t pc, int64_t regs[]) {
   ttyLocker ttyl;
-  FlagSetting fs(Debugging, true);
+  DebuggingContext debugging{};
   tty->print_cr("rip = 0x%016lx", (intptr_t)pc);
 #ifndef PRODUCT
   tty->cr();

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

@@ -628,7 +628,9 @@ 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::is_error_reported())  return nullptr;
+    if (DebuggingContext::is_enabled() || VMError::is_error_reported()) {
+      return nullptr;
+    }
     Unimplemented();
   } else if (old_gen()->is_in_reserved(addr)) {
     assert(old_gen()->is_in(addr),

src/hotspot/share/oops/accessBackend.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2023, 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
@@ -209,7 +209,7 @@ namespace AccessInternal {
 
 #ifdef ASSERT
   void check_access_thread_state() {
-    if (VMError::is_error_reported() || Debugging) {
+    if (VMError::is_error_reported() || DebuggingContext::is_enabled()) {
       return;
     }
 

src/hotspot/share/runtime/javaThread.cpp

@@ -376,6 +376,9 @@ void JavaThread::check_possible_safepoint() {
 }
 
 void JavaThread::check_for_valid_safepoint_state() {
+  // Don't complain if running a debugging command.
+  if (DebuggingContext::is_enabled()) return;
+
   // Check NoSafepointVerifier, which is implied by locks taken that can be
   // shared with the VM thread.  This makes sure that no locks with allow_vm_block
   // are held.

src/hotspot/share/runtime/mutexLocker.cpp

@@ -171,6 +171,7 @@ static int _num_mutex;
 
 #ifdef ASSERT
 void assert_locked_or_safepoint(const Mutex* lock) {
+  if (DebuggingContext::is_enabled()) return;
   // check if this thread owns the lock (common case)
   assert(lock != nullptr, "Need non-null lock");
   if (lock->owned_by_self()) return;
@@ -181,6 +182,7 @@ void assert_locked_or_safepoint(const Mutex* lock) {
 
 // a weaker assertion than the above
 void assert_locked_or_safepoint_weak(const Mutex* lock) {
+  if (DebuggingContext::is_enabled()) return;
   assert(lock != nullptr, "Need non-null lock");
   if (lock->is_locked()) return;
   if (SafepointSynchronize::is_at_safepoint()) return;
@@ -190,6 +192,7 @@ void assert_locked_or_safepoint_weak(const Mutex* lock) {
 
 // a stronger assertion than the above
 void assert_lock_strong(const Mutex* lock) {
+  if (DebuggingContext::is_enabled()) return;
   assert(lock != nullptr, "Need non-null lock");
   if (lock->owned_by_self()) return;
   fatal("must own lock %s", lock->name());

src/hotspot/share/utilities/debug.cpp

@@ -76,8 +76,16 @@ static intx g_asserting_thread = 0;
 static void* g_assertion_context = nullptr;
 #endif // CAN_SHOW_REGISTERS_ON_ASSERT
 
-// Set to suppress secondary error reporting.
+int DebuggingContext::_enabled = 0; // Initially disabled.
-bool Debugging = false;
+
+DebuggingContext::DebuggingContext() {
+  _enabled += 1;                // Increase nesting count.
+}
+
+DebuggingContext::~DebuggingContext() {
+  assert(is_enabled(), "Debugging nesting confusion");
+  _enabled -= 1;                // Decrease nesting count.
+}
 
 #ifndef ASSERT
 #  ifdef _DEBUG
@@ -166,7 +174,6 @@ static void print_error_for_unit_test(const char* message, const char* detail_fm
 
 void report_vm_error(const char* file, int line, const char* error_msg, const char* detail_fmt, ...)
 {
-  if (Debugging) return;
   va_list detail_args;
   va_start(detail_args, detail_fmt);
   void* context = nullptr;
@@ -188,7 +195,6 @@ void report_vm_status_error(const char* file, int line, const char* error_msg,
 }
 
 void report_fatal(VMErrorType error_type, const char* file, int line, const char* detail_fmt, ...) {
-  if (Debugging) return;
   va_list detail_args;
   va_start(detail_args, detail_fmt);
   void* context = nullptr;
@@ -208,7 +214,6 @@ void report_fatal(VMErrorType error_type, const char* file, int line, const char
 
 void report_vm_out_of_memory(const char* file, int line, size_t size,
                              VMErrorType vm_err_type, const char* detail_fmt, ...) {
-  if (Debugging) return;
   va_list detail_args;
   va_start(detail_args, detail_fmt);
 
@@ -278,13 +283,11 @@ void report_java_out_of_memory(const char* message) {
 
 class Command : public StackObj {
  private:
-  ResourceMark rm;
+  ResourceMark _rm;
-  bool debug_save;
+  DebuggingContext _debugging;
  public:
   static int level;
   Command(const char* str) {
-    debug_save = Debugging;
-    Debugging = true;
     if (level++ > 0)  return;
     tty->cr();
     tty->print_cr("\"Executing %s\"", str);
@@ -292,7 +295,6 @@ class Command : public StackObj {
 
   ~Command() {
     tty->flush();
-    Debugging = debug_save;
     level--;
   }
 };

src/hotspot/share/utilities/debug.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2023, 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
@@ -46,6 +46,98 @@ bool handle_assert_poison_fault(const void* ucVoid, const void* faulting_address
 #define TOUCH_ASSERT_POISON
 #endif // CAN_SHOW_REGISTERS_ON_ASSERT
 
+// The DebuggingContext class provides a mechanism for temporarily disabling
+// asserts and various consistency checks.  Ordinarily that would be a really
+// bad idea, but it's essential for some of the debugging commands provided by
+// HotSpot.  (See the Command class in debug.cpp.) These commands are intended
+// to be invoked from the debugger while the program is otherwise stopped.
+// The commands may invoke operations while the program is in a state where
+// those operations are not normally permitted, with the state checked by an
+// assert.  We want the debugging commands to bypass those checks.
+class DebuggingContext {
+  static int _enabled;          // Nesting counter.
+
+public:
+  DebuggingContext();
+  ~DebuggingContext();
+  // Asserts and other code use this to determine whether to bypass checks
+  // that would otherwise lead to program termination.
+  static bool is_enabled() { return _enabled > 0; }
+};
+
+// VMASSERT_CHECK_PASSED(P) provides the mechanism by which DebuggingContext
+// disables asserts.  It returns true if P is true or DebuggingContext is
+// enabled.  Assertion failure is reported if it returns false, terminating
+// the program.
+//
+// The DebuggingContext check being enabled isn't placed inside the report
+// function, as that would prevent the report function from being noreturn.
+// The report function should be noreturn so there isn't a control path to the
+// assertion's continuation that has P being false.  Otherwise, the compiler
+// might logically split the continuation to include that path explicitly,
+// possibly leading to discovering (and warning about) invalid code.  For
+// example, if P := x != nullptr, and the continuation contains a dereference
+// of x, the compiler might warn because there is a control path (!P -> report
+// -> continuation) where that dereference is known to be invalid.  (Of
+// course, if execution actually took that path things would go wrong, but
+// that's the risk the DebuggingContext mechanism takes.)
+//
+// Similarly, the check for enabled DebuggingContext shouldn't follow P.
+// Having this macro expand to `P || DebuggingContext::is_enabled()` has the
+// same problem of a control path through !P to the assertion's continuation.
+//
+// But it can't be just `DebuggingContext::is_enabled() || P` either.  That
+// prevents the compiler from inferring based on P that it is true in the
+// continuation.  But it also prevents the use of assertions in constexpr
+// contexts, since that expression is not constexpr.
+//
+// We could accomodate constexpr usage with std::is_constant_evaluated() (from
+// C++20). Unfortunately, we don't currently support C++20.  However, most
+// supported compilers have implemented it, and that implementation uses a
+// compiler intrinsic that we can use directly without otherwise using C++20.
+//
+// Note that if we could use std::is_constant_evaluated() then we could just
+// use this definition for DebuggingContext::is_enabled:
+//   static constexpr bool is_enabled() {
+//     return !std::is_constant_evaluated() && _enabled;
+//   }
+// The idea being that we are definitely not executing for debugging if doing
+// constant evaluation in the compiler. We don't do something like that now,
+// because we need a fallback when we don't have any mechanism for detecting
+// constant evaluation.
+#if defined(TARGET_COMPILER_gcc) || defined(TARGET_COMPILER_xlc)
+
+// gcc10 added both __has_builtin and __builtin_is_constant_evaluated.
+// clang has had __has_builtin for a long time, so likely also in xlclang++.
+// Similarly, clang has had __builtin_is_constant_evaluated for a long time.
+
+#ifdef __has_builtin
+#if __has_builtin(__builtin_is_constant_evaluated)
+#define VMASSERT_CHECK_PASSED(p) \
+  ((! __builtin_is_constant_evaluated() && DebuggingContext::is_enabled()) || (p))
+#endif
+#endif
+
+#elif defined(TARGET_COMPILER_visCPP)
+
+// std::is_constant_evaluated() and it's associated intrinsic are available in
+// VS 2019 16.5.  The minimum supported version of VS 2019 is already past
+// that, so we can rely on the intrinsic being available.
+#define VMASSERT_CHECK_PASSED(p) \
+  ((! __builtin_is_constant_evaluated() && DebuggingContext::is_enabled()) || (p))
+
+#endif // End dispatch on TARGET_COMPILER_xxx
+
+// If we don't have a way to detect constant evaluation, then fall back to the
+// less than ideal form of the check, and hope it works.  This succeeds at
+// least for gcc.  The support needed to use the above definition was added in
+// gcc10. The problems arising from analyzing the failed P case don't seem to
+// appear until gcc12.  An alternative is to not provide DebuggingContext
+// support for such a configuration.
+#ifndef VMASSERT_CHECK_PASSED
+#define VMASSERT_CHECK_PASSED(p) ((p) || DebuggingContext::is_enabled())
+#endif
+
 // assertions
 #ifndef ASSERT
 #define vmassert(p, ...)
@@ -56,10 +148,9 @@ bool handle_assert_poison_fault(const void* ucVoid, const void* faulting_address
 // compiler can't handle an empty ellipsis in a macro without a warning.
 #define vmassert(p, ...)                                                       \
 do {                                                                           \
-  if (!(p)) {                                                                  \
+  if (! VMASSERT_CHECK_PASSED(p)) {                                            \
     TOUCH_ASSERT_POISON;                                                       \
     report_vm_error(__FILE__, __LINE__, "assert(" #p ") failed", __VA_ARGS__); \
-    BREAKPOINT;                                                                \
   }                                                                            \
 } while (0)
 #endif
@@ -82,14 +173,13 @@ do {                                                                           \
 // like "Invalid argument", "out of memory" etc
 #define vmassert_status(p, status, msg) \
 do {                                                                           \
-  if (!(p)) {                                                                  \
+  if (! VMASSERT_CHECK_PASSED(p)) {                                            \
     TOUCH_ASSERT_POISON;                                                       \
     report_vm_status_error(__FILE__, __LINE__, "assert(" #p ") failed",        \
                            status, msg);                                       \
-    BREAKPOINT;                                                                \
   }                                                                            \
 } while (0)
-#endif
+#endif // ASSERT
 
 // For backward compatibility.
 #define assert_status(p, status, msg) vmassert_status(p, status, msg)
@@ -97,12 +187,12 @@ do {                                                                           \
 // guarantee is like vmassert except it's always executed -- use it for
 // cheap tests that catch errors that would otherwise be hard to find.
 // guarantee is also used for Verify options.
+// guarantee is not subject to DebuggingContext bypass.
 #define guarantee(p, ...)                                                         \
 do {                                                                              \
   if (!(p)) {                                                                     \
     TOUCH_ASSERT_POISON;                                                          \
     report_vm_error(__FILE__, __LINE__, "guarantee(" #p ") failed", __VA_ARGS__); \
-    BREAKPOINT;                                                                   \
   }                                                                               \
 } while (0)
 
@@ -110,35 +200,30 @@ do {
 do {                                                                              \
   TOUCH_ASSERT_POISON;                                                            \
   report_fatal(INTERNAL_ERROR, __FILE__, __LINE__, __VA_ARGS__);                  \
-  BREAKPOINT;                                                                     \
 } while (0)
 
 // out of memory
 #define vm_exit_out_of_memory(size, vm_err_type, ...)                             \
 do {                                                                              \
   report_vm_out_of_memory(__FILE__, __LINE__, size, vm_err_type, __VA_ARGS__);    \
-  BREAKPOINT;                                                                     \
 } while (0)
 
 #define ShouldNotCallThis()                                                       \
 do {                                                                              \
   TOUCH_ASSERT_POISON;                                                            \
   report_should_not_call(__FILE__, __LINE__);                                     \
-  BREAKPOINT;                                                                     \
 } while (0)
 
 #define ShouldNotReachHere()                                                      \
 do {                                                                              \
   TOUCH_ASSERT_POISON;                                                            \
   report_should_not_reach_here(__FILE__, __LINE__);                               \
-  BREAKPOINT;                                                                     \
 } while (0)
 
 #define Unimplemented()                                                           \
 do {                                                                              \
   TOUCH_ASSERT_POISON;                                                            \
   report_unimplemented(__FILE__, __LINE__);                                       \
-  BREAKPOINT;                                                                     \
 } while (0)
 
 #define Untested(msg)                                                             \
@@ -157,25 +242,37 @@ enum VMErrorType {
   OOM_JAVA_HEAP_FATAL = 0xe0000004
 };
 
-// Set to suppress secondary error reporting.
-// Really should have a qualified name or something.
-extern bool Debugging;
-
 // error reporting helper functions
+[[noreturn]]
 void report_vm_error(const char* file, int line, const char* error_msg);
+
+[[noreturn]]
+ATTRIBUTE_PRINTF(4, 5)
 void report_vm_error(const char* file, int line, const char* error_msg,
-                     const char* detail_fmt, ...) ATTRIBUTE_PRINTF(4, 5);
+                     const char* detail_fmt, ...);
+
+[[noreturn]]
 void report_vm_status_error(const char* file, int line, const char* error_msg,
                             int status, const char* detail);
-void report_fatal(VMErrorType error_type, const char* file, int line, const char* detail_fmt, ...) ATTRIBUTE_PRINTF(4, 5);
+
+[[noreturn]]
+ATTRIBUTE_PRINTF(4, 5)
+void report_fatal(VMErrorType error_type, const char* file, int line, const char* detail_fmt, ...);
+
+[[noreturn]]
+ATTRIBUTE_PRINTF(5, 6)
 void report_vm_out_of_memory(const char* file, int line, size_t size, VMErrorType vm_err_type,
-                             const char* detail_fmt, ...) ATTRIBUTE_PRINTF(5, 6);
+                             const char* detail_fmt, ...);
-void report_should_not_call(const char* file, int line);
+
-void report_should_not_reach_here(const char* file, int line);
+[[noreturn]] void report_should_not_call(const char* file, int line);
-void report_unimplemented(const char* file, int line);
+[[noreturn]] void report_should_not_reach_here(const char* file, int line);
+[[noreturn]] void report_unimplemented(const char* file, int line);
+
+// NOT [[noreturn]]
 void report_untested(const char* file, int line, const char* message);
 
-void warning(const char* format, ...) ATTRIBUTE_PRINTF(1, 2);
+ATTRIBUTE_PRINTF(1, 2)
+void warning(const char* format, ...);
 
 #define STATIC_ASSERT(Cond) static_assert((Cond), #Cond)