Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]

[Doug Simon]

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 [v4]

[Tom Rodriguez]

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]

[Doug Simon]

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]

[Doug Simon]

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]

[Tom Rodriguez]

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 [v4]

[Doug Simon]

> 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=14641=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=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