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

[Kim Barrett]

On Wed, 29 Mar 2023 18:09:39 GMT, Coleen Phillimore <coleenp 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.

Thanks for reviews @coleenp and @dholmes-ora .

> 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?

The earlier version (from PR#12845) used `assert(is_enabled(), "...")`.
But that's vacuous, being equivalent to `assert(false, ...)`.  (Yeah, that's
obvious.)

Of course it should never fail, but that's true for a lot of these kinds of
things.   I also thought about just discarding the check entirely.

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

PR Comment: https://git.openjdk.org/jdk/pull/13199#issuecomment-1489425653
PR Review Comment: https://git.openjdk.org/jdk/pull/13199#discussion_r1152553080