On Mon, 27 Mar 2023 23:57:45 GMT, Kim Barrett <kbarr...@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
