Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
On Fri, 30 Jun 2023 15:15:16 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix warning about conversion from size_t to int - take 2 Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615145850
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Fri, 30 Jun 2023 17:30:33 GMT, Vladimir Kozlov wrote: > > > 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! - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615144598
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Fri, 30 Jun 2023 14:35:10 GMT, Doug Simon wrote: > > 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1614962751
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
On Fri, 30 Jun 2023 15:15:16 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix warning about conversion from size_t to int - take 2 Good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1507604563
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - take 2 - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/5bb3b529..11a2dad5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Thu, 29 Jun 2023 20:06:19 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into > JDK-8310829 > - [skip ci] handle pending HotSpot exception closer to site causing exception > - revert to lazy loading of VMSupport > - each exception translation failure should trigger a JVMCI event > - try harder to show nested exception during exception translation > - resolve VMSupport at bootstrap to avoid nested exception in > ExceptionTranslation::doit I have fixed the warning on Windows: 5bb3b529d36c906ac861e5ebf1b861dbb35bfe2c > 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? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1614746764
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v7]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/e46a6a17..5bb3b529 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Thu, 29 Jun 2023 20:06:19 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into > JDK-8310829 > - [skip ci] handle pending HotSpot exception closer to site causing exception > - revert to lazy loading of VMSupport > - each exception translation failure should trigger a JVMCI event > - try harder to show nested exception during exception translation > - resolve VMSupport at bootstrap to avoid nested exception in > ExceptionTranslation::doit 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 - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1613932794
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Thu, 29 Jun 2023 02:13:32 GMT, David Holmes wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1613736704
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
> 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. Doug Simon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8310829 - [skip ci] handle pending HotSpot exception closer to site causing exception - revert to lazy loading of VMSupport - each exception translation failure should trigger a JVMCI event - try harder to show nested exception during exception translation - resolve VMSupport at bootstrap to avoid nested exception in ExceptionTranslation::doit - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/9236128a..e46a6a17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=04-05 Stats: 13222 lines in 537 files changed: 6305 ins; 3442 del; 3475 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Wed, 28 Jun 2023 21:21:11 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > [skip ci] handle pending HotSpot exception closer to site causing exception 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. - PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1504459576
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Wed, 28 Jun 2023 21:21:11 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > [skip ci] handle pending HotSpot exception closer to site causing exception That's more clear to me. Thanks! - Marked as reviewed by never (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1504385398
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Wed, 28 Jun 2023 18:28:37 GMT, Tom Rodriguez wrote: > Why can't the pending exception handling be in the respective virtual? Done: 9236128ad1b7297c8c4e9d3022b88c3ab3278022 - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1612113760
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: [skip ci] handle pending HotSpot exception closer to site causing exception - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/734903a8..9236128a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=03-04 Stats: 145 lines in 3 files changed: 102 ins; 25 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 21:29:53 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > revert to lazy loading of VMSupport 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1611888908
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:06:49 GMT, Tom Rodriguez wrote: > 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). - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610903263
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:08:22 GMT, Tom Rodriguez wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert to lazy loading of VMSupport
>
> src/hotspot/share/jvmci/jvmciEnv.cpp line 402:
>
>> 400: }
>> 401: int res = encode(THREAD, buffer, buffer_size);
>> 402: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1244794019
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 21:29:53 GMT, Doug Simon wrote:
>> 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.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert to lazy loading of VMSupport
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);
}
src/hotspot/share/jvmci/jvmciEnv.cpp line 402:
> 400: }
> 401: int res = encode(THREAD, buffer, buffer_size);
> 402: 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?
-
PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610335668
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1244461845
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610243648
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: revert to lazy loading of VMSupport - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/819a24fd..734903a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=02-03 Stats: 35 lines in 5 files changed: 17 ins; 2 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608455121
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:56:21 GMT, David Holmes wrote: > 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 - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608388679
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:47:08 GMT, Doug Simon wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608371203
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:41:25 GMT, David Holmes wrote: > 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.` would ever fail in practice. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608330027
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:06:46 GMT, Doug Simon wrote: > 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? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608306762
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event looks ok to me. - Marked as reviewed by never (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1498967746
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> 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. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event BTW, I've manually tested this with libgraal. It's not possible to add a jtreg test until libgraal in part of OpenJDK. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607466267
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: each exception translation failure should trigger a JVMCI event - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/614e1e9f..819a24fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=01-02 Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
On Mon, 26 Jun 2023 07:59:17 GMT, David Holmes wrote: > 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.` 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. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607433634
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
> 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. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: try harder to show nested exception during exception translation - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/fafc0aae..614e1e9f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=00-01 Stats: 51 lines in 4 files changed: 21 ins; 4 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote: > 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. Edited above to correct: I think it is just highlighting that this code **not** is prepared for "system" errors like this. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606923008
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Mon, 26 Jun 2023 07:46:38 GMT, Doug Simon wrote: > 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 prepared for "system" errors like this. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606915981
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Mon, 26 Jun 2023 07:42:48 GMT, David Holmes wrote:
> 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 usages of `vmSupport` below all use `JavaCalls:call_static` which takes
care of linking and initializing the class.
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 585:
>
>> 583:
>> 584: if (class_name->utf8_length() <= 1) {
>> 585: 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.
-
PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606899412
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1241766196
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote:
> 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.
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. ??
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 585:
> 583:
> 584: if (class_name->utf8_length() <= 1) {
> 585: JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should
> be handled in Java code", str));
Seems unrelated to the fix at hand.
-
PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1497840467
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1241749015
