Re: RFR: 8264711: More runtime TRAPS cleanups

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264711: More runtime TRAPS cleanups

Lois Foltan-3
On Mon, 5 Apr 2021 17:57:13 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>
> Thanks, Harold

Looks good Harold.  One minor comment for the file jfrJavaSupport.cpp.

Thanks,
Lois

src/hotspot/share/jfr/jni/jfrJavaSupport.cpp line 144:

> 142:   ObjectSynchronizer::jni_enter(h_obj, THREAD->as_Java_thread());
> 143:   ObjectSynchronizer::notifyall(h_obj, THREAD);
> 144:   ObjectSynchronizer::jni_exit(THREAD->as_Java_thread(), h_obj());

For consistency can you switch the parameter order for jni_enter as well in this change?  It looks a little bit odd that jni_exit was changed and not jni_enter.

-------------

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3345
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264711: More runtime TRAPS cleanups

David Holmes
On 6/04/2021 5:14 am, Patricio Chilano Mateo wrote:
> src/hotspot/share/prims/jni.cpp line 2738:
>
>> 2736:
>> 2737:   Handle obj(THREAD, JNIHandles::resolve_non_null(jobj));
>> 2738:   ObjectSynchronizer::jni_exit(THREAD->as_Java_thread(), obj());
>
> Here we would return JNI_ERR if we throw IMSE from jni_exit().

Strictly speaking we probably should return JNI_ERR in that case, but
the spec (as usual) is non-specific about the relationship between error
codes and throwing exceptions. I would not suggest making such a change
now. Note that we would have to be careful to only return JNI_ERR in the
single case of IMSE, and even then we would have to be certain that the
IMSE came from the actual "exit" and not e.g.

err = MonitorEnter(obj);
...
throwIMSE()
...
err = MonitorExit(obj)

Cheers,
David

> src/hotspot/share/runtime/synchronizer.cpp line 609:
>
>> 607:   // intentionally do not use CHECK on check_owner because we must exit the
>> 608:   // monitor even if an exception was already pending.
>> 609:   if (monitor->check_owner(current)) {
>
> We can actually throw IMSE from check_owner() if this thread is not the real owner.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3345
>