RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

[Kim Barrett]

On Feb 8, 2017, at 5:13 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:

> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04

> src/share/vm/runtime/javaCalls.cpp
>    L526:                 "signature does not matched pushed arguments: %u", state);
>        Typo: 'matched' -> 'match'
> 
>    For the guarantees on L522 and L525, would the (_pos - 1) value
>    be useful for diagnostic purposes?

Done.

>    L569:       if (v != 0 ) {
>        Not your bug, but there's an extra blank before the ")".

Done.

>    L571:         if (t < (size_t)os::vm_page_size()) {
>        Again, not your issue, but that size < page size check means
>        the oop is bad is screaming for a short comment.

Added comment.  Also removed the unnecessary intermediate variable t,
thereby eliminating some casts.  I'm not completly convinced by the
comment, but remaining true to the existing code.  Here's the diff:

@@ -566,9 +568,10 @@
     uint value_state = _value_state[p];
     if (is_value_state_indirect_oop(value_state)) {
       intptr_t v = _value[p];
-      if (v != 0 ) {
-        size_t t = (size_t)v;
-        if (t < (size_t)os::vm_page_size()) {
+      if (v != 0) {
+        if (v < os::vm_page_size()) {
+          // v is a "handle" referring to an oop, cast to integral type.
+          // There ought not be any handles in very low memory.
           bad = true;
         } else if (!resolve_indirect_oop(v, value_state)->is_oop_or_null(true)) {
           bad = true;

> src/share/vm/runtime/jniHandles.cpp
>    (old) L115     assert(!CheckJNICalls || is_weak_global_handle(handle), "Invalid delete of weak global JNI handle");
> 
>        Don't like the old assert? Is is_weak_global_handle() now obsolete?

There is an is_jweak assert in jweak_ref.  That's cheap (unlike
is_weak_global_handle), so not conditional on CheckJNICalls.

There are still a few calls to is_weak_global_handle.  They all occur
as alternatives with is_global_handle &etc, so I was reluctant to
replace them with is_jweak (which would also require making that
public).

> src/cpu/s390/vm/templateInterpreterGenerator_s390.cpp
>    (old) L1705:   __ verify_oop(Rlresult);
>        The delete of the verify_oop() call after the store_oop_result
>        label threw me for a few minutes. Then I realized that the
>        only uses of the store_oop_result label appear to be when we
>        have a NULL oop. New code is cleaner.

There's another, after the G1 block.  But it's preceeded by a
verify_oop of its own.

> src/cpu/sparc/vm/templateInterpreterGenerator_sparc.cpp
>    Not your bug, but unlike sharedRuntime_sparc.cpp, there are no
>    verify_oop calls here.
> 
> src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp
>    Not your bug, but unlike sharedRuntime_x86{32,64}.cpp,
>    there are no verify_oop calls here.

I didn't add them where they weren't already present.  I could be
convinced to change that.

> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>    jcheck will likely complain about the empty line at the end
>    of the file.
> 
> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>    Same empty line at the end of the file comment.

Okay.  I don't use jcheck until preparing to push, because it doesn't
understand mq patches.

> Thumbs up! If you choose to fix any of the nits above, I don't
> need to see another webrev.

Thanks, Dan.