8302189: Mark assertion failures noreturn #12845

kimbarrett · 2023-03-03T04:04:25Z

Also 8302799: Refactor Debugging variable usage for noreturn crash reporting
/issue add JDK-8302799

Please review this change to make the functions used to report assertion
failures and the like be noreturn, and mark them as such for the compiler
using the [[noreturn]] attribute.

These functions were returning if the Debugging variable was true, and must
not do so anymore.

We introduce a new mechanism for temporarily disabling assertions while
calling certain "debugging" commands while stopped in a debugger:
DebuggingContext. This replaces the Debugging variable.

To disable assertions when in a debugging context the various checking macros
now also check for the context. This ends up being a bit complicated and
messy. See discussion in debug.hpp.

Only error reporting under the control of ASSERT is affected by the debugging
context. Others seem much less likely to be survivable. Explicit debugging
checks can be added where needed to support the debugging commands. A few
are included with this change. There might be more needed; I haven't
exhaustively tested the debugging commands.

In some respects it would be better to not have assertions be automatically
disabled by DebuggingContext, and instead use explicit context checks where
needed. The problem is that "where needed" is a difficult to determine and
maintain set.

Testing:
mach5 tier1-7

Manually tested some of the debugging commands.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8302189: Mark assertion failures noreturn
JDK-8302799: Refactor Debugging variable usage for noreturn crash reporting

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to f296ab62
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to f296ab62

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12845/head:pull/12845
$ git checkout pull/12845

Update a local copy of the PR:
$ git checkout pull/12845
$ git pull https://git.openjdk.org/jdk pull/12845/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12845

View PR using the GUI difftool:
$ git pr show -t 12845

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12845.diff

bridgekeeper · 2023-03-03T04:05:22Z

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-03T04:07:33Z

@kimbarrett
Adding additional issue to issue list: 8302799: Refactor Debugging variable usage for noreturn crash reporting.

openjdk · 2023-03-03T04:09:19Z

@kimbarrett The following labels will be automatically applied to this pull request:

build
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk · 2023-03-03T04:09:46Z

@kimbarrett Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2023-03-03T04:12:58Z

Webrevs

kimbarrett · 2023-03-03T04:18:36Z

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


I forgot to mention this in the PR summary. New warning suppression for
4722: 'function' : destructor never returns, potential memory leak
We have various destructors calling ShouldNotCallThis, ShouldNotReachHere,
or the like, which triggers this warning. It might be possible to deal with
some (but not all) of those by deleting the destructor. The warning doesn't
seem all that useful to us, so I just suppressed it.

dholmes-ora

A few initial comments while I try to digest this.

I don't use these particular debugging mechanism, neither Debugger nor BREAKPOINT, but it seems potentially problematic to me that the removed BREAKPOINTs happened after the error was reported, and IIUC the new mechanism will activate before the error is reported.

Thanks.

dholmes-ora · 2023-03-03T04:15:40Z

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


It is annoying that we don't document what these warnings are. :(

I've made the same comment in the past. This time I decided to do something about it:
https://bugs.openjdk.org/browse/JDK-8303618
Document disabled C/C++ warnings on Windows

Looks like something I can help with, will assign the enhancement to myself

dholmes-ora · 2023-03-03T04:26:04Z

src/hotspot/share/utilities/debug.cpp

-  ResourceMark rm;
-  bool debug_save;
+  ResourceMark _rm;
+  DebuggingContext _debugging;


Why a different initialization syntax here?

I don't understand the question? These are member variable declarations.

I believe David is asking about why these particular member names were changed

No I'm comparing the syntax used elsewhere:

DebuggingContext debugging{};

with that used here:

DebuggingContext debugging;

why the braces in most cases but not here?

"syntax used elsewhere" are declarations of local variables initialized with value initialization (which is default initialization for this type). "that used here" is a member variable declaration in the Command class, where the initialization occurs elsewhere (in the constructor of the Command class [*]). They are entirely different syntactic entities. Does that help? Or am I still not understanding your question?

[*] With C++11, data members can have default initializers, but that's not an approved feature for HotSpot.

Sorry I was fixated on the weirdness (to me) of the {} initialization. I missed the fact these were different kinds of declarations.

dholmes-ora · 2023-03-03T04:27:15Z

src/hotspot/share/utilities/debug.cpp

+  if (is_enabled()) {
+    fatal("Multiple Debugging contexts");
+  }


This seems too restrictive as you could hit different DebuggingContexts in different threads. ??

This facility is only intended for use by manually invoked commands while the program is stopped in a debugger. Multi-threaded use is not an issue (and was not supported previously either). I don't think there are any nested uses either, but I've now run across a couple of places where nesting could be useful. So I'm changing the state from a simple bool to a nesting counter.

Okay I hadn't realized the process was basically "suspended" when this was activated - as I said I never use this stuff.

kimbarrett · 2023-03-04T11:16:50Z

I don't use these particular debugging mechanism, neither Debugger nor BREAKPOINT, but it seems potentially problematic to me that the removed BREAKPOINTs happened after the error was reported, and IIUC the new mechanism will activate before the error is reported.

The removed BREAKPOINTs were not happening after the error was reported.
Normally the report functions would report and die, without reaching the
BREAKPOINT at all. They were only reachable when "Debugging" was enabled by
being in one of the manually invoked "commands", which disabled the report
function and made it return immediately to the BREAKPOINT, which isn't really
very interesting because in one of these commands we want to just "ignore"
errors entirely.

dholmes-ora

Okay other than the one outstanding minor syntax query, I have nothing further to add.

Thanks.

openjdk · 2023-03-07T01:38:03Z

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8302189: Mark assertion failures noreturn
8302799: Refactor Debugging variable usage for noreturn crash reporting

Reviewed-by: dholmes, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

coleenp

Seems fine. I'll test drive with the debug commands next time I need to use them but this is similar to to the changes I have in my private repos. Thanks for keeping this functionality.

coleenp · 2023-03-07T13:08:22Z

src/hotspot/share/utilities/debug.hpp

+// 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)


All this seems like it should go in COMPILER_HEADER(globalDefinitions.hpp) but since globalDefinitions.hpp includes debug.hpp, you can't do this. Can we file an RFE to clean this up (if possible)?

I considered that, and had two reasons for rejecting it.

The include ordering problem was one. There are uses of assert and the like in
globalDefinitions. Some of those might someday be moved elsewhere, but for now
the order is a problem.

The other is that this is really not "general purpose", but rather very
specifically tailored to the use here. And it can't be general purpose,
because there isn't a general fallback for testing for constexpr evaluation.

I could have added HAS_IS_CONSTANT_EVALUATED and ::is_constant_evaluated(),
with all uses of the latter requiring protection by the former. But I couldn't
think of any places where I would want to use that. Using it in, for example,
count_trailing_zeros to allow it to be (sometimes) constexpr would make the
question of whether it's constexpr be platform-dependent, which means it can't
be used in shared required constexpr contexts. And it's even worse since the
platform dependency is also currently compiler version dependent. I think that
path leads nowhere good.

However, I've filed this cleanup RFE:
https://bugs.openjdk.org/browse/JDK-8303797

kimbarrett · 2023-03-08T02:33:45Z

Thanks for reviews @dholmes-ora and @coleenp

kimbarrett · 2023-03-08T02:33:56Z

/integrate

openjdk · 2023-03-08T02:37:13Z

Going to push as commit 5fa9bd4.

openjdk · 2023-03-08T02:37:30Z

@kimbarrett Pushed as commit 5fa9bd4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

noreturn attributes

894d93a

openjdk bot added the rfr label Mar 3, 2023

new implementation of Debugging

ca68e50

kimbarrett force-pushed the noreturn2 branch from f1b78f8 to ca68e50 Compare March 3, 2023 04:08

openjdk bot added hotspot build labels Mar 3, 2023

kimbarrett commented Mar 3, 2023

View reviewed changes

dholmes-ora reviewed Mar 3, 2023

View reviewed changes

make Debugging::_enabled a nesting counter

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

f296ab6

dholmes-ora approved these changes Mar 7, 2023

View reviewed changes

openjdk bot added the ready label Mar 7, 2023

coleenp approved these changes Mar 7, 2023

View reviewed changes

Merge branch 'master' into noreturn2

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

5929561

openjdk bot added the integrated label Mar 8, 2023

openjdk bot closed this Mar 8, 2023

openjdk bot removed ready rfr labels Mar 8, 2023

kimbarrett deleted the noreturn2 branch March 8, 2023 06:51

kimbarrett mentioned this pull request Mar 27, 2023

8303805: [REDO] JDK-8302189 and JDK-8302799 #13199

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8302189: Mark assertion failures noreturn #12845

8302189: Mark assertion failures noreturn #12845

kimbarrett commented Mar 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

mlbridge bot commented Mar 3, 2023 •

edited

Loading

kimbarrett Mar 3, 2023

dholmes-ora left a comment

dholmes-ora Mar 3, 2023

kimbarrett Mar 4, 2023

TheShermanTanker Mar 5, 2023

dholmes-ora Mar 3, 2023

kimbarrett Mar 4, 2023

TheShermanTanker Mar 5, 2023

dholmes-ora Mar 7, 2023

kimbarrett Mar 7, 2023

dholmes-ora Mar 7, 2023

dholmes-ora Mar 3, 2023

kimbarrett Mar 4, 2023

dholmes-ora Mar 7, 2023

kimbarrett commented Mar 4, 2023

dholmes-ora left a comment

openjdk bot commented Mar 7, 2023 •

edited

Loading

coleenp left a comment

coleenp Mar 7, 2023 •

edited

Loading

kimbarrett Mar 8, 2023 •

edited

Loading

kimbarrett commented Mar 8, 2023

kimbarrett commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

8302189: Mark assertion failures noreturn #12845

8302189: Mark assertion failures noreturn #12845

Conversation

kimbarrett commented Mar 3, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

mlbridge bot commented Mar 3, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimbarrett commented Mar 4, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 7, 2023 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

coleenp Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kimbarrett Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

kimbarrett commented Mar 8, 2023

kimbarrett commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

kimbarrett commented Mar 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 3, 2023 •

edited

Loading

openjdk bot commented Mar 7, 2023 •

edited

Loading

coleenp Mar 7, 2023 •

edited

Loading

kimbarrett Mar 8, 2023 •

edited

Loading