8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit #14641

dougxc · 2023-06-25T06:58:14Z

The VMSupport class is required for translating an exception between the HotSpot and libgraal heaps.
Loading it lazily can result in a loading exception, obscuring the exception being translated.
To avoid this, VMSupport is loaded eagerly along with the other vmClasses.

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-8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit (Bug - P2)

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to e46a6a17
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14641

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

Using diff file

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

Webrev

Link to Webrev Comment

…Translation::doit

bridgekeeper · 2023-06-25T06:59:10Z

👋 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.

openjdk · 2023-06-25T07:00:46Z

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

graal
hotspot
serviceability

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.

mlbridge · 2023-06-25T07:05:03Z

Webrevs

dholmes-ora

The eager loading seems reasonable, but I do not understand the details here. In what way was loading failing? You still have to initialize VMSupport before you can call methods on it, so that could also fail - though the code does not seem to notice/handle this. ??

dholmes-ora · 2023-06-26T07:31:05Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

@@ -582,7 +582,7 @@ C2V_VMENTRY_NULL(jobject, lookupType, (JNIEnv* env, jobject, jstring jname, ARGU
  TempNewSymbol class_name = SymbolTable::new_symbol(str);

  if (class_name->utf8_length() <= 1) {
-    JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should be handled in Java code", class_name->as_C_string()));
+    JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should be handled in Java code", str));


Seems unrelated to the fix at hand.

Yes, it's a minor fix up I noticed while making changes a few lines below. It just avoids a conversion of a Symbol back to a C string when the original C string is available.

dougxc · 2023-06-26T07:46:38Z

The eager loading seems reasonable, but I do not understand the details here. In what way was loading failing? You still have to initialize VMSupport before you can call methods on it, so that could also fail - though the code does not seem to notice/handle this. ??

The failure was due to OutOfMemoryError when running a GC test that stresses the heap (TestPLABEvacuationFailure.java).
The usages of vmSupport below all use JavaCalls:call_static which takes care of linking and initializing the class.

dholmes-ora · 2023-06-26T07:59:17Z

The usages of vmSupport below all use JavaCalls:call_static which takes care of linking and initializing the class.

Sure but my point is that initialization can fail and the code does not seem to directly take that into account - if the first decode encounters a class initialization error then it will just return with the exception pending and no decoding will actually have occurred. If we get to the encode first and it encounters an initialization error it will still attempt to do a decode that must in turn fail and again we return with an exception pending and no encoding/decoding occurring. This code does not seem to be written in a way that allows for an error initializing VMSupport. So while the fix side-steps the problematic guarantee, I think it is just highlighting that this code is not prepared for "system" errors like this.

dholmes-ora · 2023-06-26T08:02:43Z

Edited above to correct: I think it is just highlighting that this code is not prepared for "system" errors like this.

dougxc · 2023-06-26T13:06:46Z

if the first decode encounters a class initialization error then it will just return with the exception pending and no decoding will actually have occurred

That's fine - if VMSupport fails class initialization, then no "rich" decoding can occur (by definition) and so throwing the clinit error is the best we can do.

If we get to the encode first and it encounters an initialization error it will still attempt to do a decode that must in turn fail

You have to keep in mind that encode and decode are called in different runtimes. If encoding an exception in the HotSpot runtime fails (e.g. due to an OOME calling VMSupport.<clinit> in the HotSpot heap), then it's still fine to try call VMSupport.decode in the libgraal runtime. If VMSupport.decode in the libgraal runtime also fails, then it throw the exception describing the secondary failure. There's simply no way to guarantee all info is shown when secondary issues occur during exception handling.

dougxc · 2023-06-26T13:21:52Z

BTW, I've manually tested this with libgraal. It's not possible to add a jtreg test until libgraal in part of OpenJDK.

tkrodriguez

looks ok to me.

openjdk · 2023-06-26T16:44:16Z

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

8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit

Reviewed-by: never, kvn

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

e8ff74c: 8310987: Missing @SInCE tag(s) in java/util/logging/ErrorManager.java
e3a7e02: 8311162: Simplify and modernize equals and hashCode for java.net
430d6b6: 8309976: Add microbenchmark for stressing code cache
33011ea: 8310946: G1: Refactor G1Policy::next_gc_should_be_mixed
a7d168b: 8311026: Some G1 specific tests do not set -XX:+UseG1GC
c08c983: 8311000: missing @SInCE info in jdk.management
660cd25: 8310988: Missing @SInCE tags in java.management.rmi
2a9e2f6: 8311046: ProblemList gc/z/TestHighUsage.java with Generational ZGC
77e7b6d: 8311074: RISC-V: Fix -Wconversion warnings in some code header files
11fd34e: 8310241: OffsetDateTime compareTo redundant computation
... and 1 more: https://git.openjdk.org/jdk/compare/a995aa6cd1cd89c2ca6db72b060177eaf7065377...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.

dholmes-ora · 2023-06-26T21:41:25Z

That's fine - if VMSupport fails class initialization, then no "rich" decoding can occur (by definition) and so throwing the clinit error is the best we can do.

Then why did you not simply handle the exception from the loading of VMSupport the same way?

dougxc · 2023-06-26T21:47:08Z

Then why did you not simply handle the exception from the loading of VMSupport the same way?

The only actual case seen of the original way failing is due to OOME in GC stress tests. If loading of VMSupport is done during VM startup, then no stress test code can cause an OOME while loading VMSupport. I doubt VMSupport.<clinit> would ever fail in practice.

dholmes-ora · 2023-06-26T21:56:21Z

I doubt VMSupport. would ever fail in practice.

I would think it is likely to fail with OOME under the same GC stress test conditions.

But if this is just a stress test issue then bailing out when the loading fails would be far simpler than pre-loading the class. You've added to the VM startup cost just to avoid a stress test failure.

dougxc · 2023-06-26T22:10:42Z

I would think it is likely to fail with OOME under the same GC stress test conditions.

But if this is just a stress test issue then bailing out when the loading fails would be far simpler than pre-loading the class. You've added to the VM startup cost just to avoid a stress test failure.

Any app running near the GC limit can cause this and we've seen this with libgraal in practice. When the VM is near the GC limit, any compilation can cause an OOME and then when that OOME wants to be translated back to libgraal, it can be the first time VMSupport is used.
The time for loading VMSupport eagerly is in the noise.
For example, -verbose:class shows that about 30 classes (including VMSupport) are loaded in 1ms:

[0.011s][info][class,load] java.lang.reflect.Executable source: shared objects file
[0.012s][info][class,load] java.lang.reflect.Method source: shared objects file
[0.012s][info][class,load] java.lang.reflect.Constructor source: shared objects file
[0.012s][info][class,load] jdk.internal.vm.ContinuationScope source: shared objects file
[0.012s][info][class,load] jdk.internal.vm.Continuation source: shared objects file
[0.012s][info][class,load] jdk.internal.vm.StackChunk source: shared objects file
[0.012s][info][class,load] jdk.internal.vm.VMSupport source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.MagicAccessorImpl source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.MethodAccessor source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.MethodAccessorImpl source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.DelegatingClassLoader source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.ConstantPool source: shared objects file
[0.012s][info][class,load] java.lang.annotation.Annotation source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.CallerSensitive source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessor source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessorImpl source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.DirectConstructorHandleAccessor$NativeAccessor source: shared objects file
[0.012s][info][class,load] jdk.internal.reflect.SerializationConstructorAccessorImpl source: shared objects file
[0.012s][info][class,load] java.lang.invoke.MethodHandle source: shared objects file
[0.012s][info][class,load] java.lang.invoke.DirectMethodHandle source: shared objects file
[0.012s][info][class,load] java.lang.invoke.VarHandle source: shared objects file
[0.012s][info][class,load] java.lang.invoke.MemberName source: shared objects file
[0.012s][info][class,load] java.lang.invoke.ResolvedMethodName source: shared objects file
[0.012s][info][class,load] java.lang.invoke.MethodHandleNatives source: shared objects file
[0.012s][info][class,load] java.lang.invoke.LambdaForm source: shared objects file
[0.012s][info][class,load] java.lang.invoke.TypeDescriptor$OfMethod source: shared objects file
[0.012s][info][class,load] java.lang.invoke.MethodType source: shared objects file
[0.012s][info][class,load] java.lang.BootstrapMethodError source: shared objects file
[0.012s][info][class,load] java.lang.invoke.CallSite source: shared objects file
[0.012s][info][class,load] jdk.internal.foreign.abi.NativeEntryPoint source: shared objects file
[0.012s][info][class,load] jdk.internal.foreign.abi.ABIDescriptor source: shared objects file
[0.012s][info][class,load] jdk.internal.foreign.abi.VMStorage source: shared objects file
[0.013s][info][class,load] jdk.internal.foreign.abi.UpcallLinker$CallRegs source: shared objects file

dholmes-ora · 2023-06-26T23:21:25Z

It may be in the noise but noise adds up over time.

It just seems to me that the simplest fix here would have been to convert

Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, THREAD);
guarantee(!HAS_PENDING_EXCEPTION, "");

to

Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, CHECK);

and just return on exception. A very isolated change with zero impact on anything else.

dougxc · 2023-06-27T21:27:55Z

I've pushed the loading of VMSupport down into the 2 cases where it's actually needed and made it lazy again which should address all concerns about extra noise at VM startup. Thanks for the push back on this - I agree that it's the best solution.

tkrodriguez · 2023-06-27T23:06:49Z

I don't think pushing it down that far improves things. encode always precedes decode so why not resolve it before the encode call.

      // Resolve VMSupport class explicitly as HotSpotJVMCI::compute_offsets
      // may not have been called.
      Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, THREAD);
      int res = 0;
      if (!HAS_PENDING_EXCEPTION) {
        res = encode(THREAD, vmSupport, buffer, buffer_size);
      }

tkrodriguez · 2023-06-27T23:08:22Z

src/hotspot/share/jvmci/jvmciEnv.cpp

        return;
      }
-      int res = encode(THREAD, vmSupport, buffer, buffer_size);
+      int res = encode(THREAD, buffer, buffer_size);
      if (_from_env != nullptr && !_from_env->is_hotspot() && _from_env->has_pending_exception()) {


Why is this check before the HAS_PENDING_EXCEPTION check? Couldn't you end up returning with a pending exception in this path?

If encode is SharedLibraryToHotSpotExceptionTranslation::encode there is no possibility of a pending HotSpot exception upon it returning. If it's HotSpotToSharedLibraryExceptionTranslation::encode then _from_env->is_hotspot() is true and so this if branch is not taken.

dougxc · 2023-06-28T07:23:08Z

I don't think pushing it down that far improves things. encode always precedes decode so why not resolve it before the encode call.

Because the VMSupport klass is only needed if calling into HotSpot so it's better to push it down into HotSpotToSharedLibraryExceptionTranslation::encode. Also, if the VMSupport klass is used for encoding, it's not needed for decoding (the libgraal VMSupport jclass value is used instead).

tkrodriguez · 2023-06-28T18:28:37Z

Right, I was forgetting about the virtual nature of this code. Why can't the pending exception handling be in the respective virtual? The partition between virtual and shared is kind of ugly.

…eption

dougxc · 2023-06-28T21:15:12Z

Why can't the pending exception handling be in the respective virtual?

Done: 9236128

tkrodriguez

That's more clear to me. Thanks!

dholmes-ora

This is now isolated to JVMCI code that I'm not familiar with, so don't feel competent to review. The general idea seems okay but I'm not familiar enough with the details. Someone from the compiler team should review this now.

Thanks.

…8310829

dougxc · 2023-06-29T20:01:00Z

Someone from the compiler team should review this now.

@vnkozlov could you please review this or nominate someone else from the compiler team to look at it. Thanks.

vnkozlov · 2023-06-29T23:56:44Z

I am fins with idea of changes. But, please, activate GHA testing for this branch.
And there is build error on Windows:

c:\workspace\open\src\hotspot\share\jvmci\jvmciEnv.cpp(449): error C2220: the following warning is treated as an error
c:\workspace\open\src\hotspot\share\jvmci\jvmciEnv.cpp(449): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data

dougxc · 2023-06-30T14:35:10Z

I have fixed the warning on Windows: 5bb3b52

But, please, activate GHA testing for this branch.

Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's the point of doing redundant testing?

vnkozlov · 2023-06-30T17:30:33Z

But, please, activate GHA testing for this branch.

Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's the point of doing redundant testing?

It builds and tests configurations (32-bit) we don't have in our testing.

vnkozlov

Good.

dougxc · 2023-06-30T20:08:23Z

But, please, activate GHA testing for this branch.

Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's the point of doing redundant testing?

It builds and tests configurations (32-bit) we don't have in our testing.

Good to know - thanks!

dougxc · 2023-06-30T20:09:39Z

Thanks for the reviews.

/integrate

openjdk · 2023-06-30T20:10:27Z

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

456bf11: 8310173: Update --release 21 symbol information for JDK 21 build 29
19601eb: 8310993: Missing @SInCE tags in jdk.attach
e8ff74c: 8310987: Missing @SInCE tag(s) in java/util/logging/ErrorManager.java
e3a7e02: 8311162: Simplify and modernize equals and hashCode for java.net
430d6b6: 8309976: Add microbenchmark for stressing code cache
33011ea: 8310946: G1: Refactor G1Policy::next_gc_should_be_mixed
a7d168b: 8311026: Some G1 specific tests do not set -XX:+UseG1GC
c08c983: 8311000: missing @SInCE info in jdk.management
660cd25: 8310988: Missing @SInCE tags in java.management.rmi
2a9e2f6: 8311046: ProblemList gc/z/TestHighUsage.java with Generational ZGC
... and 3 more: https://git.openjdk.org/jdk/compare/a995aa6cd1cd89c2ca6db72b060177eaf7065377...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-30T20:10:38Z

@dougxc Pushed as commit f6bdccb.

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

resolve VMSupport at bootstrap to avoid nested exception in Exception…

fafc0aa

…Translation::doit

openjdk bot added the rfr Pull request is ready for review label Jun 25, 2023

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jun 25, 2023

dholmes-ora reviewed Jun 26, 2023

View reviewed changes

try harder to show nested exception during exception translation

614e1e9

each exception translation failure should trigger a JVMCI event

819a24f

tkrodriguez approved these changes Jun 26, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 26, 2023

revert to lazy loading of VMSupport

734903a

tkrodriguez reviewed Jun 27, 2023

View reviewed changes

[skip ci] handle pending HotSpot exception closer to site causing exc…

9236128

…eption

tkrodriguez approved these changes Jun 28, 2023

View reviewed changes

dholmes-ora reviewed Jun 29, 2023

View reviewed changes

[skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-…

e46a6a1

…8310829

fix warning about conversion from size_t to int

5bb3b52

fix warning about conversion from size_t to int - take 2

11a2dad

vnkozlov approved these changes Jun 30, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 30, 2023

openjdk bot closed this Jun 30, 2023

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

8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit #14641

8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit #14641

Conversation

dougxc commented Jun 25, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 25, 2023

openjdk bot commented Jun 25, 2023

mlbridge bot commented Jun 25, 2023 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jun 26, 2023

Choose a reason for hiding this comment

dougxc Jun 26, 2023

Choose a reason for hiding this comment

dougxc commented Jun 26, 2023 • edited Loading

dholmes-ora commented Jun 26, 2023 • edited Loading

dholmes-ora commented Jun 26, 2023 • edited Loading

dougxc commented Jun 26, 2023

dougxc commented Jun 26, 2023

tkrodriguez left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 26, 2023 • edited Loading

dholmes-ora commented Jun 26, 2023

dougxc commented Jun 26, 2023

dholmes-ora commented Jun 26, 2023

dougxc commented Jun 26, 2023

dholmes-ora commented Jun 26, 2023

dougxc commented Jun 27, 2023

tkrodriguez commented Jun 27, 2023

tkrodriguez Jun 27, 2023

Choose a reason for hiding this comment

dougxc Jun 28, 2023

Choose a reason for hiding this comment

dougxc commented Jun 28, 2023

tkrodriguez commented Jun 28, 2023

dougxc commented Jun 28, 2023

tkrodriguez left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dougxc commented Jun 29, 2023

vnkozlov commented Jun 29, 2023

dougxc commented Jun 30, 2023

vnkozlov commented Jun 30, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

dougxc commented Jun 30, 2023

dougxc commented Jun 30, 2023

openjdk bot commented Jun 30, 2023

openjdk bot commented Jun 30, 2023

dougxc commented Jun 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 25, 2023 •

edited

Loading

dougxc commented Jun 26, 2023 •

edited

Loading

dholmes-ora commented Jun 26, 2023 •

edited

Loading

dholmes-ora commented Jun 26, 2023 •

edited

Loading

openjdk bot commented Jun 26, 2023 •

edited

Loading