Skip to content
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

8269416: [JVMCI] capture libjvmci crash data to a file #4620

Closed
wants to merge 4 commits into from

Conversation

dougxc
Copy link
Member

@dougxc dougxc commented Jun 28, 2021

When a fatal error occurs in libgraal, it writes a crash dump to tty. Instead, it should be captured in a separate log file that is then referenced in the HotSpot crash summary (just like the hs_err_pid and CI replay compile log files are). This allows libgraal crash data to be more easily submitted along with VM crash reports.

For example:

> java -Dlibgraal.CrashAtIsFatal=true -Dgraal.CrashAt=String.equals -cp bin CountUppercase skjdf
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (jvmciRuntime.cpp:909), pid=36298, tid=41219
#  fatal error: thread 41219: Fatal error in JVMCI shared library
#
# JRE version: OpenJDK Runtime Environment GraalVM LIBGRAAL 21.3.0-dev (16.0.2) (build 16.0.2-internal+0-adhoc.dnsimon.labsjdk-ce-16)
# Java VM: OpenJDK 64-Bit Server VM GraalVM LIBGRAAL 21.3.0-dev (16.0.2-internal+0-adhoc.dnsimon.labsjdk-ce-16, mixed mode, tiered, jvmci, jvmci compiler, compressed oops, compressed class ptrs, g1 gc, bsd-amd64)
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /Users/dnsimon/graal/graal/compiler/hs_err_pid36298.log
#
# The JVMCI shared library error data is saved as:
# /Users/dnsimon/graal/graal/compiler/hs_err_pid36298_libjvmci.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269416: [JVMCI] capture libjvmci crash data to a file

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4620/head:pull/4620
$ git checkout pull/4620

Update a local copy of the PR:
$ git checkout pull/4620
$ git pull https://git.openjdk.java.net/jdk pull/4620/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4620

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4620.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 28, 2021

👋 Welcome back dnsimon! 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.

@dougxc dougxc marked this pull request as ready for review Jun 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@dougxc The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Jun 28, 2021
@dougxc dougxc changed the title [JVMCI] capture libjvmci crash data to a file 8269416: [JVMCI] capture libjvmci crash data to a file Jun 29, 2021
@openjdk openjdk bot added the rfr label Jun 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 29, 2021

Webrevs

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jun 29, 2021

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label Jun 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@vnkozlov
The hotspot-runtime label was successfully added.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Someone from Runtime group may have good suggestions for this changes.
I think you need to follow (or create JVMCI specific flags) flags used for error reporting:

SuppressFatalErrorMessage
ShowMessageBoxOnError
UseOSErrorReporting
ErrorFileToStdout
ErrorFileToStderr

@@ -1584,6 +1587,13 @@ void VMError::report_and_die(int id, const char* message, const char* detail_fmt
}
}

#if INCLUDE_JVMCI
if (JVMCI::fatal_log_filename() != NULL) {
out.print_raw("#\n# The JVMCI shared library error data is saved as:\n# ");
Copy link
Contributor

@vnkozlov vnkozlov Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer report file instead of data.

@dougxc
Copy link
Member Author

@dougxc dougxc commented Jun 29, 2021

Someone from Runtime group may have good suggestions for this changes.
I think you need to follow (or create JVMCI specific flags) flags used for error reporting:

SuppressFatalErrorMessage

I don't think extra handling is needed for that flag. It cuts off all paths that lead to libjvmci error handling.

ErrorFileToStdout
ErrorFileToStderr

I will push a change to respect these flags in JVMCI::fatal_log.

ShowMessageBoxOnError
UseOSErrorReporting

It's not clear to me that I need to worry about these - advice welcome.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Okay. this is fine.

Still you need review from Runtime group. I will ping them.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@dougxc 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:

8269416: [JVMCI] capture libjvmci crash data to a file

Reviewed-by: kvn, dholmes

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 32 new commits pushed to the master branch:

  • a0f32cb: 8268906: gc/g1/mixedgc/TestOldGenCollectionUsage.java assumes that GCs take 1ms minimum
  • ee0247f: 8263461: jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java uses wrong mechanism to cause evacuation failure
  • 3ad20fc: 8269571: NMT should print total malloc bytes and invocation count
  • b969136: 8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load
  • ee526a2: Merge
  • 0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
  • d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
  • 401cb0a: 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
  • b8a16e9: 8268884: C2: Compile::remove_speculative_types must iterate top-down
  • 25f9f19: 8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/87ff27786b7310840aab00c391e2a7fa19a3c328...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 29, 2021
@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jun 29, 2021

Personally I think JVMCI error reporting can be handled independently of the other VM error reporting flags etc. No need to interact with ErrorFileToStdOut/Err as this is not the error file that flag refers to.

I will take a look at the changes in more detail.

Thanks,
David

Copy link
Member

@dholmes-ora dholmes-ora left a comment

This is predominantly a compiler change so I can only approve the shared runtime changes in vmError.

I have no concerns about this change from a runtime perspective.

Thanks,
David

@dougxc
Copy link
Member Author

@dougxc dougxc commented Jun 30, 2021

Thanks for the review David. If it's all the same, I'll keep the ErrorFileToStdOut/Err behavior for this libjvmci error reporting. Looking at the rationale for https://bugs.openjdk.java.net/browse/JDK-8220786, it makes sense for libjvmci error reporting to go to the console when retrieving log files from container environments is problematic. It might even be worth having CI compiler replay respect these flags but that's a separate question that I leave up to @vnkozlov.

@dougxc
Copy link
Member Author

@dougxc dougxc commented Jun 30, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

Going to push as commit a6b253d.
Since your change was applied there have been 32 commits pushed to the master branch:

  • a0f32cb: 8268906: gc/g1/mixedgc/TestOldGenCollectionUsage.java assumes that GCs take 1ms minimum
  • ee0247f: 8263461: jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java uses wrong mechanism to cause evacuation failure
  • 3ad20fc: 8269571: NMT should print total malloc bytes and invocation count
  • b969136: 8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load
  • ee526a2: Merge
  • 0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
  • d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
  • 401cb0a: 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
  • b8a16e9: 8268884: C2: Compile::remove_speculative_types must iterate top-down
  • 25f9f19: 8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/87ff27786b7310840aab00c391e2a7fa19a3c328...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jun 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

@dougxc Pushed as commit a6b253d.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 30, 2021

Mailing list message from David Holmes on hotspot-dev:

On 30/06/2021 5:17 pm, Doug Simon wrote:

On Tue, 29 Jun 2021 22:14:50 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

Doug Simon has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:

respect ErrorFileToStdout and ErrorFileToStderr flags

Okay. this is fine.

Still you need review from Runtime group. I will ping them.

Thanks for the review David. If it's all the same, I'll keep the ErrorFileToStdOut/Err behavior for this libjvmci error reporting. Looking at the rationale for https://bugs.openjdk.java.net/browse/JDK-8220786, it makes sense for libjvmci error reporting to go to the console when retrieving log files from container environments is problematic. It might even be worth having CI compiler replay respect these flags but that's a separate question that I leave up to @vnkozlov.

No problem. I hadn't seen that you'd made the change when I made the
comment.

David

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 30, 2021

Mailing list message from David Holmes on hotspot-dev:

On 30/06/2021 5:17 pm, Doug Simon wrote:

On Tue, 29 Jun 2021 22:14:50 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

Doug Simon has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:

respect ErrorFileToStdout and ErrorFileToStderr flags

Okay. this is fine.

Still you need review from Runtime group. I will ping them.

Thanks for the review David. If it's all the same, I'll keep the ErrorFileToStdOut/Err behavior for this libjvmci error reporting. Looking at the rationale for https://bugs.openjdk.java.net/browse/JDK-8220786, it makes sense for libjvmci error reporting to go to the console when retrieving log files from container environments is problematic. It might even be worth having CI compiler replay respect these flags but that's a separate question that I leave up to @vnkozlov.

No problem. I hadn't seen that you'd made the change when I made the
comment.

David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-runtime integrated
3 participants