8308151: [JVMCI] capture JVMCI exceptions in hs-err #14000

dougxc · 2023-05-16T08:02:11Z

When there is a pending exception after a JVMCI upcall into libjvmci, the VM calls the ExceptionDescribe JNI function to print the exception. Unfortunately, this output goes to "a system error-reporting channel" [1] which may not be tty. It also means the output is not in a hs-err log should the VM then exit with a fatal error. This has historically made it harder to triage libgraal bugs (i.e. the console output is usually required in addition to the hs-err crash log).

This PR addresses these shortcomings by printing the exception info to a string which is added to the JVMCI event log (for hs-err):

JVMCI Events (11 events):
...
Event: 0.274 Thread 0x0000000146819210 compiler.jvmci.TestUncaughtErrorInCompileMethod$CompilerCreationError
Event: 0.274 Thread 0x0000000146819210  at compiler.jvmci.TestUncaughtErrorInCompileMethod$1.createCompiler(TestUncaughtErrorInCompileMethod.java:147)
Event: 0.274 Thread 0x0000000146819210  at jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.getCompiler(HotSpotJVMCIRuntime.java:829)
Event: 0.274 Thread 0x0000000146819210  at jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.compileMethod(HotSpotJVMCIRuntime.java:943)

It is also be used to enhance the -XX:+PrintCompilation message issued for a failed compilation:

COMPILE SKIPPED: uncaught exception in call_HotSpotJVMCIRuntime_compileMethod [compiler.jvmci.TestUncaughtErrorInCompileMethod$CompilerCreationError]

[1] https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#exceptiondescribe

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

Issue

JDK-8308151: [JVMCI] capture JVMCI exceptions in hs-err

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to b1854a6a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14000

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-16T08:03:50Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into pr/13905 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-05-16T08:06:20Z

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

graal
hotspot-compiler

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.

dougxc · 2023-05-16T09:05:25Z

src/hotspot/share/jvmci/jvmciRuntime.cpp

  JVMCIObject result_object = JVMCIENV->call_HotSpotJVMCIRuntime_compileMethod(receiver, jvmci_method, entry_bci,
                                                                     (jlong) compile_state, compile_state->task()->compile_id());
+#ifdef ASSERT
+  if (JVMCIENV->has_pending_exception() && JVMCICompileMethodExceptionIsFatal) {


It's a shame to introduce a VM flag (i.e., JVMCICompileMethodExceptionIsFatal) for a test case but I don't know of any other way to do this. As far as I know, system properties cannot be accessed here. Maybe using an environment variable is better than a VM flag?

Why can't you use a JVMCI property for this? You get a chance to see them when copying them to Graal.

The copying is only done when using libgraal. I'd like to have this test run in a JDK without libgraal.

I see that I can simply use system properties after all: 90f4346

Yes that looks good.

mlbridge · 2023-05-16T09:09:00Z

Webrevs

tkrodriguez · 2023-05-16T16:38:02Z

src/hotspot/share/jvmci/jvmciEnv.cpp

+      line++;
+    }
+    if (line >= max_lines) {
+      JVMCI_event_1("[elided %d more stack trace lines]", line - max_lines);


You could add this output to the last line instead of burning an extra line.

Indeed:

Event: 0.237 Thread 0x000000011e019e10 compiler.jvmci.TestUncaughtErrorInCompileMethod$CompilerCreationError Event: 0.237 Thread 0x000000011e019e10 at compiler.jvmci.TestUncaughtErrorInCompileMethod$1.createCompiler(TestUncaughtErrorInCompileMethod.java:161) [elided 2 more stack trace lines]

tkrodriguez · 2023-05-16T16:43:01Z

src/hotspot/share/jvmci/jvmciRuntime.cpp

  JVMCIObject result_object = JVMCIENV->call_HotSpotJVMCIRuntime_compileMethod(receiver, jvmci_method, entry_bci,
                                                                     (jlong) compile_state, compile_state->task()->compile_id());
+#ifdef ASSERT
+  if (JVMCIENV->has_pending_exception() && JVMCICompileMethodExceptionIsFatal) {


Why can't you use a JVMCI property for this? You get a chance to see them when copying them to Graal.

…mpileMethodExceptionIsFatal system property

dougxc · 2023-05-22T12:23:35Z

src/hotspot/share/jvmci/jvmciRuntime.cpp

  JVMCIObject result_object = JVMCIENV->call_HotSpotJVMCIRuntime_compileMethod(receiver, jvmci_method, entry_bci,
                                                                     (jlong) compile_state, compile_state->task()->compile_id());
+  if (JVMCIENV->has_pending_exception()) {
+    const char* val = Arguments::PropertyList_get_value(Arguments::system_properties(), "test.jvmci.compileMethodExceptionIsFatal");


Note that this view on system properties is restricted to properties set at VM startup (e.g. on the command line) and will not see the result of calls to System.setProperty() made by an application.

openjdk-notifier · 2023-05-23T07:25:57Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8308151
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-05-23T07:27:27Z

@dougxc this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8308151
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-05-23T08:43:18Z

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

8308151: [JVMCI] capture JVMCI exceptions in hs-err

Reviewed-by: never

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

e9320f3: 8308116: jdk.test.lib.compiler.InMemoryJavaCompiler.compile does not close files
97d3b27: 8307523: [vectorapi] Optimize MaskFromLongBenchmark.java
bb0ff48: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM
c0c4d77: 8308544: Fix compilation regression from JDK-8306983 on musl libc
9e196b3: 8308565: HttpClient: Sanitize logging while stopping
582ddeb: 8308545: java/net/httpclient/ShutdownNow.java fails with "stream 1 cancelled"
1cfb265: 8307814: In the case of two methods with Record Patterns, the second one contains a line number from the first method
eb11508: 8308281: Java snippets in the FFM API need to be updated
26227a6: 8305073: Fix VerifyLoopOptimizations - step 2 - verify idom
80d7de7: 8305582: Compiler crash when compiling record patterns with var
... and 4 more: https://git.openjdk.org/jdk/compare/69f508a2ac344eb61cef7be985348873b8265171...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 · 2023-05-23T15:42:15Z

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

tkrodriguez · 2023-05-24T06:55:50Z

The latest version looks ok to me.

dougxc · 2023-05-24T07:14:04Z

Thanks Tom for the reviews.

/integrate

openjdk · 2023-05-24T07:15:20Z

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

beb75e6: 8306302: C2 Superword fix: use VectorMaskCmp and VectorBlend instead of CMoveVF/D
2836c34: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts
8ffa264: 8306698: Add overloads to MethodTypeDesc::of
6b27dad: 8301154: SunPKCS11 KeyStore deleteEntry results in dangling PrivateKey entries
ed0e956: 8308716: ProblemList java/util/concurrent/ScheduledThreadPoolExecutor/BasicCancelTest.java with genzgc on windows-x64
bddf483: 8303942: os::write should write completely
ab241b3: 8306706: Support out-of-line code generation for MachNodes
710453c: 8308016: Use snippets in java.io package
e9320f3: 8308116: jdk.test.lib.compiler.InMemoryJavaCompiler.compile does not close files
97d3b27: 8307523: [vectorapi] Optimize MaskFromLongBenchmark.java
... and 12 more: https://git.openjdk.org/jdk/compare/69f508a2ac344eb61cef7be985348873b8265171...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-24T07:15:34Z

@dougxc Pushed as commit 05c095c.

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

dougxc added 3 commits May 10, 2023 16:23

make JMCI more robust in low resource conditions

26a1c42

remove unused callToString method

348b3ae

send JVMCI exception info to hs-err log and/or tty

b1854a6

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels May 16, 2023

dougxc marked this pull request as ready for review May 16, 2023 09:01

openjdk bot added the rfr Pull request is ready for review label May 16, 2023

dougxc commented May 16, 2023

View reviewed changes

tkrodriguez approved these changes May 16, 2023

View reviewed changes

dougxc added 2 commits May 16, 2023 23:14

append elision comment to end of last stack trace line

9efd3ea

replace JVMCICompileMethodExceptionIsFatal VM flag with test.jvmci.co…

90f4346

…mpileMethodExceptionIsFatal system property

dougxc mentioned this pull request May 18, 2023

8306992: [JVMCI] mitigate more against JVMCI related OOME causing VM to exit #13905

Closed

3 tasks

dougxc commented May 22, 2023

View reviewed changes

openjdk-notifier bot changed the base branch from pr/13905 to master May 23, 2023 07:25

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label May 23, 2023

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8308151

fb50cbb

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 23, 2023

[skip ci] make TestUncaughtErrorInCompileMethod more robust

0471e7c

dougxc force-pushed the JDK-8308151 branch from fe2ca69 to 0471e7c Compare May 23, 2023 15:33

openjdk bot added the ready Pull request is ready to be integrated label May 23, 2023

openjdk bot added the rfr Pull request is ready for review label May 23, 2023

openjdk bot added the integrated Pull request has been integrated label May 24, 2023

openjdk bot closed this May 24, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 24, 2023

dougxc deleted the JDK-8308151 branch August 20, 2024 06:20

8308151: [JVMCI] capture JVMCI exceptions in hs-err #14000

8308151: [JVMCI] capture JVMCI exceptions in hs-err #14000

Uh oh!

Conversation

dougxc commented May 16, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 16, 2023

Uh oh!

openjdk bot commented May 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk-notifier bot commented May 23, 2023

Uh oh!

openjdk bot commented May 23, 2023

Uh oh!

openjdk bot commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 23, 2023

Uh oh!

tkrodriguez commented May 24, 2023

Uh oh!

dougxc commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

dougxc commented May 16, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 16, 2023 •

edited

Loading

openjdk bot commented May 23, 2023 •

edited

Loading