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