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

[Thomas Schatzl]

Hi Kim,

On Mon, 2017-02-06 at 20:03 -0500, Kim Barrett wrote:
> Apologies for taking so long to respond to earlier comments.  I've
> been distracted by some other tasks, and it took a while to figure
> out what to do about the problem Mikael reported.
> 
> 
[...]
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166188
> 
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04

 - ReturnJNIWeak.java: please make sure that the test is not called
with -XX:+ExplicitGCInvokesConcurrent. It will most likely fail in this
case.

 - jniHandles.hpp: it might be useful to explain the external_guard
template parameter to the new guard_value/resolve_impl methods.

This might be due to my unfamiliarity to the code, so skip this if you
think it is "obvious" what this means.

>From the callers of JNIHandles::resolve() I am actually not sure if the
extra performance due to templatizing the parameter has any impact.

Again, your call.

 - it would be nice if JNIHandles::weak_oops_do(BoolObjectClosure,
OopClosure) would have an assert_at_safepoint_or_JNI_lock_locked() -
but that's probably another issue. Same with the _global_handles array.

 - jniHandles.hpp:58. Not sure why the "int" type specifier for
weak_tag_value has an extra space in front of it.

 - isn't is_global_handle() thread-unsafe too? The assert uses
JNIHandleBlock::chain_contains(), which iterates over the JNIHandles
array unguarded, but it seems possible to add entries concurrently
using e.g. make_global()?

I.e. one thread adding a global handle, another thread calling e.g.
jni_GetObjectRefType?

That may be another issue. Same with the accesses to
_weak_global_handle value.

 - javaCalls.hpp: in line 135 I would prefer if the two increment
statements were put on separate lines. I looked over the second one a
few times too many.

 - javaCalls.cpp:449: s/Invaild/Invalid/

 - javaCalls.cpp:526: s/matched/match/ ; maybe also upper case the
initial word of the sentence.

 - javaCalls.cpp: somebody already asked for a comment for the t <
os::vm_page_size() check.

 - in SignatureChekker::check_obj(), maybe put the declaration of the
"bad" bool into the if-scope as it is never used outside.

 - I would prefer that the tagging mechanism would be described better
in JNIHandles.hpp. "Low tag bit in jobject used to distinguish a jweak"
seems not sufficient to explain what and why we do the tagging (or how
it works).
Particularly because some code (shareNativeWrapper.cpp) refers to an
explanation in JNIHandles.hpp which is not there.

 - templateInterpreterGenerator_sparc.cpp, line 1523: am I correct that
in case of jweaks, this generates a guaranteed unaligned load?

E.g. the sequence:

brx(Assembler::zero, true, Assembler::pt, store_result);
delayed()->ld_ptr(O0, 0, O0);  // <--- this one I think is a guaranteed
unaligned load if O0 contains a jweak.
ld_ptr(O0, -JNIHandles::weak_tag_value, O0);

On some machines this may cause a SIGBUS afaik - depending on OS
configuration/capabilities. I would prefer if the code would not
attempt to be clever here.

Apparently the same for sharedRuntime_sparc.cpp.

 - I am also not sure why some of the code generation methods have the
STATIC_ASSERT(JNIHandles::weak_tag_mask == 1u) and others not. (E.g.
ARM32/64 code has it, others apparently not).
I would kind of prefer to have it everywhere, but I am also fine with
one e.g. in JNIHandles.cpp with a big fat comment.

Thanks,
  Thomas