On Wed, 29 Mar 2023 18:09:39 GMT, Coleen Phillimore <[email protected]> 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
