-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8266404: Fatal error report generated with -XX:+CrashOnOutOfMemoryError should not contain suggestion to submit a bug report #4006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…or should not contain suggestion to submit a bug report
|
👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into |
|
@dholmes-ora The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
/label remove hotspot |
|
@dholmes-ora |
|
@dholmes-ora |
Webrevs
|
tstuefe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David,
mostly fine, minor nits.
Side note, I really would like to clean up this coding a bit. I have attempted to so so multiple times, but the patches ballooned. I may try again, maybe in tiny baby steps.
Cheers, Thomas
tstuefe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good.
|
@dholmes-ora 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: 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 87 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Hi David, looks good. I get it now I've found that should_report_bug() really means "is vm internal error". Literally, "is this NOT a native memory allocation failure". If you had time to rename should_report_bug() "is_internal_error" or similar, I think that would make vmError.cpp significantly more readable. (It's not a problem in the function you're adding, it's the old should_report_bug that seems misnamed, and more so now it has a similar related function.) |
|
Hi Kevin,
My preference would be to keep the patches as concise as possible, because that makes backporting patches easier. vmError is overdue for some cleanups, but I'd prefer those concentrated in a cleanup RFE, which would give us just one commit to packport, instead of spreading cleanups over unrelated patches. Just my 5c, David may see this differently. Cheers, Thomas |
|
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Thomas, Thanks for looking at this. On 13/05/2021 3:51 pm, Thomas Stuefe wrote:
Unfortunately no, that is why they are both present and commented.
No reason to expose. Changed to static.
Ooops! Well spotted. I've no idea why these are expressed this way
?? These are the ones that are meant to be exposed. We hide behind the Thanks, |
1 similar comment
|
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Thomas, Thanks for looking at this. On 13/05/2021 3:51 pm, Thomas Stuefe wrote:
Unfortunately no, that is why they are both present and commented.
No reason to expose. Changed to static.
Ooops! Well spotted. I've no idea why these are expressed this way
?? These are the ones that are meant to be exposed. We hide behind the Thanks, |
|
Mailing list message from Thomas Stüfe on hotspot-runtime-dev: Hi David, On Thu, May 13, 2021 at 10:04 AM David Holmes <david.holmes at oracle.com>
This enum, signal numbers (posix) and windows SEH codes are mashed together I don't like this design though. I would like to keep these as two separate
Yes, but atm the sole user of this api lives in debug.cpp. Otherwise this Never mind though, it is not that important.
Patch looks good to me. Cheers, Thomas |
1 similar comment
|
Mailing list message from Thomas Stüfe on hotspot-runtime-dev: Hi David, On Thu, May 13, 2021 at 10:04 AM David Holmes <david.holmes at oracle.com>
This enum, signal numbers (posix) and windows SEH codes are mashed together I don't like this design though. I would like to keep these as two separate
Yes, but atm the sole user of this api lives in debug.cpp. Otherwise this Never mind though, it is not that important.
Patch looks good to me. Cheers, Thomas |
| void report_fatal(const char* file, int line, const char* detail_fmt, ...) { | ||
| va_list detail_args; | ||
| va_start(detail_args, detail_fmt); | ||
| report_fatal_impl(INTERNAL_ERROR, file, line, detail_fmt, detail_args); | ||
| va_end(detail_args); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this API? Can't we just update:
#define fatal(...) \
do { \
TOUCH_ASSERT_POISON; \
report_fatal(INTERNAL_ERROR, __FILE__, __LINE__, __VA_ARGS__); \
BREAKPOINT; \
} while (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the fatal() macro seems to be the only user of the existing report_fatal then yes I think we can simplify this as you suggest. I had thought it was used directly from elsewhere.
Thanks for the suggestion.
|
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Kevin, Thanks for looking at this. On 14/05/2021 8:18 pm, Kevin Walls wrote:
I couldn't determine exactly what "should_report_error" was really Thanks, |
|
Looks still good. ..Thomas |
|
Mailing list message from David Holmes on hotspot-runtime-dev: Thanks Thomas! David On 17/05/2021 3:44 pm, Thomas Stuefe wrote: |
gerard-ziemski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
|
Thanks Gerard! |
|
/integrate |
|
@dholmes-ora Since your change was applied there have been 92 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit cd1c17c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
A simple fix to add a new VMErrorType (OOM_JAVA_HEAP_FATAL) to indicate we've requested a Java OOM error to be fatal. A new overload of report_fatal is provided to allow this to be passed through.
VMError has an existing notion of "should_report_bug" but that encompasses more than just printing the bug submission URL, so I added a new specific check for that.
Checked hs_err files before and after with the fix, from the CrashOnOutOfMemoryError test.
Tested tiers 1-3 for good measure.
Thanks,
David
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4006/head:pull/4006$ git checkout pull/4006Update a local copy of the PR:
$ git checkout pull/4006$ git pull https://git.openjdk.java.net/jdk pull/4006/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4006View PR using the GUI difftool:
$ git pr show -t 4006Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4006.diff