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

[Andrew Haley]

On 19/01/17 05: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/
> 
> 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.
> 

Looks OK except for a small typo.  I've done very light testing, but this
edge case is very hard to test anyway.

Missing semicolon:

@@ -1403,10 +1403,33 @@
     __ adr(t, ExternalAddress(AbstractInterpreter::result_handler(T_OBJECT)));
     __ cmp(t, result_handler);
     __ br(Assembler::NE, no_oop);
-    // retrieve result
+    // Unbox oop result, e.g. JNIHandles::resolve result.
     __ pop(ltos);
     __ cbz(r0, store_result);
-    __ ldr(r0, Address(r0, 0));
+#if !INCLUDE_ALL_GCS
+    assert(!JNIHandles::need_tagged_jweaks(), "Unexpected configuration");
+#else
+    if (JNIHandles::need_tagged_jweaks()) {
+      Label not_weak;
+      STATIC_ASSERT(JNIHandles::weak_tag_mask == 1u);
+      __ tbz(r0, 0, not_weak);     // Test for weak tag.
+      // Load from tagged jweak.
+      __ ldr(r0, Address(r0, -JNIHandles::weak_tag_value));

+      // Generate the G1 pre-barrier code to log the jweak's value.
+      assert(UseG1GC, "Unsupported use of tagged jweaks");
+      __ enter();               // Barrier may call runtime.
+      __ g1_write_barrier_pre(noreg /* obj */,
+                              r0 /* pre_val */,
+                              rthread /* thread */,
+                              t /* tmp */,
+                              true /* tosca_live */,
+                              true /* expand_call */);
+      __ leave();
+      __ b(store_result);
+      __ bind(not_weak);
+    }
+#endif // INCLUDE_ALL_GCS
+    __ ldr(r0, Address(r0, 0))  // Get oop from box.

                               ^   HERE


     __ bind(store_result);
     __ str(r0, Address(rfp, frame::interpreter_frame_oop_temp_offset*wordSize));
     // keep stack depth as expected by pushing oop which will eventually be discarded