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

Mikael Gerdin mikael.gerdin at oracle.com
Thu Jan 19 16:28:27 UTC 2017


Hi Kim,

On 2017-01-19 06:31, Kim Barrett wrote:
> Please review this change to jweak to support G1.
>
> The problem being addressed is that, when using G1, dereferencing a
> jweak must ensure the obtained referent is ensured live, just as for
> other weak reference types.  This has always been a requirement, and
> failure to do so appears to be a since-day1 G1 bug.
>
> There are two categories of places that need to address this issue:
> JNIHandles::resolve and friends, and returning a jobject from native
> code to Java.  In both of these places the jobject whose contained oop
> is being extracted might really be a jweak.  jweak is
> representationally indistinguishable from jobject; jweak is just a
> typedef for jobject.  The documentation says a jweak can be used
> anywhere a jobject is permitted, making that type equivalence
> necessary for a C API.  (A C++ API might be able to have distinct
> types via inheritance, though would still need a method for
> distinguishing them at runtime.  But since JNI is a C API...).
>
> The basic approach being taken is to "tag" jweaks by setting the low
> order bit (recall that jweak == jobject == _jobject*), in order to
> distinguish them from non-jweak jobjects.  This tagging is only being
> done when the selected GC needs the distinction, e.g. G1.  This gives
> us a representational difference between jobject and jweak on which to
> base the different behavior, hiding that distinction behind the
> existing API.
>
> JNIHandles::resolve and friends are changed to unconditionally strip
> off any potential weak tag when translating from jobject to to oop*.
> That's cheaper than testing for the conditional use of tagging and
> then stripping.  Also stripped when deleting JNI handles.
>
> TemplateInterpreterGenerator::generate_native_entry and
> SharedRuntime::generate_native_wrapper are changed to conditionally
> emit code to apply the G1 pre-barrier to the value obtained from a
> jobject result, when the result is tagged as a jweak, which only
> occurs when using G1.
>
> For arm32/arm64, this required moving the g1_write_barrier_pre
> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
> moved g1_write_barrier_post, for consistency.
>
> Note: I've not tested the aarch64 changes; I'll need someone with
> access to that platform to test.
>
> Note: I've not implemented this change for ppc or s390; I'll need
> someone else to deal with those platforms.  (It's been a very long
> time since I wrote any ppc assembly code, and I've never seen an
> s390.)
>
> Note: I've done a minimal change for zero.  Code elsewhere suggests
> zero doesn't support G1, and I followed that.
>
> And finally, JNIHandles::make_weak_global conditionally tags the
> result.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166188
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.02/

I thought a bit about this issue today and unfortunately I've uncovered 
yet another location where a weak JNI handle needs to have its weak tag 
cleared.
Consider the following JNI code snippet

jobject o = <...>;
jweak w = (*env)->NewWeakGlobalRef(env, o);
jobject param = w;
(*env)->CallStaticVoidMethod(env, c, id, param);

jni_CallStaticVoidMethod creates a JNI_ArgumentPusherVaArg and passes 
that to jni_invoke_static in jni.cpp

jni_invoke_static creates a JavaCallArguments object and passes that to 
the argument pusher and invokes
   args->iterate( Fingerprinter(method).fingerprint() );

iterate calls get_object() on the args pusher which is supposed to 
transfer the object parameter from the va_list in CallStaticVoidMethod 
to the JavaCallArguments object.
The code to perform that transfer is this nasty piece of code:

inline void get_object() {
   jobject l = va_arg(_ap, jobject);
   _arguments->push_oop(Handle((oop *)l, false));
}

l here is our jweak and the code just does an unsafe cast to oop* and 
even fakes a VM Handle using a special constructor in the Handle class.

In a debug build (or with -Xcheck:jni) this results in 
JavaCallArguments::verify catching this bad oop using a 
SignatureChekker::check_obj (sic) which uses this interesting code 
snippet to verify an oop:

intptr_t v = _value[p];
if (v != 0 ) {
   size_t t = (size_t)v;
   bad = (t < (size_t)os::vm_page_size() ) ||
     !Handle::raw_resolve((oop *)v)->is_oop_or_null(true);


In a product build we will crash at some random point in the future.

I've created a test to provoke this situation and put it at:
http://cr.openjdk.java.net/~mgerdin/8166188-pass-arguments/webrev/
run the test with
$ make test-hotspot-jtreg-native

Sorry for not coming up with this sooner, I just realized this problem 
today.

/Mikael

>
> In addition to the above webrev, I've also included separate webrevs
> for a test of this change, and for a whitebox extension needed for
> that test.  The whitebox extension is my in-progress work on
> JDK-8169517, which is an enhancement targeted for JDK 10, and is not
> part of this change.  Since the test depends on that whitebox
> extension, the test can't be included with the change either, and will
> be deferred to JDK 10.  But I'm including webrevs for both here, for
> for reference by reviewers, and for testing on the platforms I don't
> have access to.
>
> http://cr.openjdk.java.net/~kbarrett/8166188/test.03/
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hsroot.01/
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hotspot.01/
>
> Testing:
> All hotspot jtreg tests run locally (Linux x86_64).
> In particular:
> - gc/TestReturnJNIWeak (new)
> - runtime/SameObject (uses NewWeakGlobalRef)
>
> Ad hoc hotspot nightly test on Oracle supported platforms.  This
> includes some closed tests that use NewWeakGlobalRef.
>


More information about the ppc-aix-port-dev mailing list