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

[Kim Barrett]

> On Feb 10, 2017, at 5:49 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> 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

Thanks for looking at this.  I’ll have a new webrev containing changes
discussed here and elsewhere soon; need to do some testing first.

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

Yes, I noticed that yesterday.  Thanks.

>  - 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.

I've added some commentary.  The template parameter is just whether
these helpers are being called from resolve_external_guard or not.
That variant resolves some erroneous cases to NULL, rather than
treating them as (possibly unchecked) errors.

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

I expect making external_guard an ordinary bool argument and letting
the compiler constant fold would produce equivalent code.  I went with
a template parameter because, semantically, my intent is that the
value must be a constant, and making it a template parameter enforces
that.

>  - 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.

Separate issue.  File an RFE?

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

— TBD

>  - 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.

Separate issue.  File a bug?

>  - 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.

Agreed.  Done.

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

Typos fixed.  signature is the name of the variable, so I prefer not capitalizing.

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

Already added.  Still not totally convinced this is appropriate.

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

Done.  Also eliminated p variable, which I don’t think helps readability.
And cleaned up the comments nearby.

>  - 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.

Added more description.

>  - 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);

The code is correct, there is no unaligned access.

In the brx, the second argument is true.  That’s the annul flag.  That means
the instruction in the delay slot will only be executed if the branch is taken,
and will be treated as a nop if the branch is not taken.  If the annul flag is
false then the instruction in the delay slot will be executed whether the branch
is taken or not.

> 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.

In non-ARM code, there’s no dependency on the size of the tag.  The ARM/AARCH64
code is using a tbz instruction, test a single bit (specifically bit 0 in this case, see the
second argument) and branch if zero.  So we’re assuming the tag field is one bit wide
and at bit zero, e.g. mask == 1.  Hence the static assert.