RFR: 8303805: [REDO] JDK-8302189 and JDK-8302799

[Coleen Phillimore]

On Mon, 27 Mar 2023 23:57:45 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this redo of the following changes:
> 8302189: Mark assertion failures noreturn
> 8302799: Refactor Debugging variable usage for noreturn crash reporting
> 
> The original change tripped over a bug the handling of noreturn and
> __builtin_unreachable by Xcode12 (so probably generic clang12 and perhaps
> earlier).  It seems to have been fixed in Xcode13 (and probably other uses of
> clang13), but we're not yet ready to require that version.
> 
> As such, we've taken the original change and kludged around the problem for
> now.  The kludge should be removed once we require clang13+.
> 
> As a reminder, the original change that was backed out did the following:
> https://github.com/openjdk/jdk/pull/12845
> 
> (1) Changed how assertions were suppressed when running the various debugging
> commands defined in debug.cpp.  The failure reporting functions used to just
> return if the Debugging variable was set (by a debug command invocation).
> Now, debug command invocation establishes a DebuggingContext object which
> causes asserts to bypass the check entirely.
> 
> (2) Added [[noreturn]] attributes to error reporting functions declared in
> debug.hpp.
> 
> This change consists of
> (1) Reapply the change from PR#12845.
> (2) A minor improvement to the DebuggingContext destructor.
> (3) Replace uses of [[noreturn]] with new ATTRIBUTE_NORETURN macro (the kludge).
> 
> The ATTRIBUTE_NORETURN macro expands to `[[noreturn]]` unless the compiler is
> clang and its version is less than 13, which it expands to nothing.
> 
> The replaced [[noreturn]] uses include both those added by JDK-8302189 and
> those added by the earlier JDK-8302798.
> 
> This is problematic in the long run.  It prevents the removal of otherwise
> obsolete and unreachable code following a call to a noreturn function.  We
> have such code in various places because these functions were previously not
> noreturn (and with this change still aren't for clang12 and prior).  It may
> also require the inclusion of such code in future changes.  This may result in
> change testing on Linux and/or Windows missing problems that will only show up
> when testing on a clang-using platform.
> 
> We're really going to want to require a minimum of clang13 sooner rather than
> later.  Unfortunately, we've run into other problems with Xcode13 and Xcode14,
> and the aix-ppc maintainers have only just begun testing xlclang++ 17.1.  (The
> version they have been using is based on clang3!)  Hence this kludge for now,
> to permit use of gcc12 (JDK-8294031).
> 
> Testing:
> mach5 tier1-7.

This seems fine to me.

src/hotspot/share/utilities/debug.cpp line 89:

> 87:     _enabled -= 1;              // Decrease nesting count.
> 88:   } else {
> 89:     fatal("Debugging nesting confusion");

Why did you make this change?  Isn't this an RAII object - how would this be a problem?

-------------

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13199#pullrequestreview-1363695920
PR Review Comment: https://git.openjdk.org/jdk/pull/13199#discussion_r1152317319