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

Tue Feb 7 13:59:16 UTC 2017

Hi Kim,

thanks for adding and adapting my code. There's only one minor bug on PPC64. Please use the correct label name for the conditional branch as shown in the diff below.

If you like, you can add simonis and mdoerr as contributors, but I don't insist on it.

I guess SignatureChekker was written with "kk" to distinguish it from something else?

Besides that, the change looks good to me.

Thanks and best regards,
Martin

--- a/src/cpu/ppc/vm/sharedRuntime_ppc.cpp      Tue Feb 07 12:48:05 2017 +0100
+++ b/src/cpu/ppc/vm/sharedRuntime_ppc.cpp      Tue Feb 07 14:30:48 2017 +0100
@@ -2483,7 +2483,7 @@
   if (ret_type == T_OBJECT || ret_type == T_ARRAY) {
     Label done, not_weak;
     __ cmpdi(CCR0, R3_RET, 0);
-    __ beq(CCR0, null_result);  // Use NULL as-is.
+    __ beq(CCR0, done);         // Use NULL as-is.

     __ andi_(r_temp_1, R3_RET, JNIHandles::weak_tag_mask);
     __ beq(CCR0, not_weak);     // Test for jweak tag.




-----Original Message-----
From: s390x-port-dev [mailto:s390x-port-dev-bounces at openjdk.java.net] On Behalf Of Kim Barrett
Sent: Dienstag, 7. Februar 2017 02:04
To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
Cc: s390x-port-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
Subject: Re: RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

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.

Martin Doerr - Thanks for the ppc and s390 native call generator modifications.  Do you want a contributed-by credit when pushed?

Andrew Haley - Thanks for the syntax fix.  The new version with always tagged jweaks (see below) makes testing easier, and there's a new test included.  Testing the G1-specific behavior still needs the whitebox extension.  I've updated the G1-specific test (which is still not part of the changeset for JDK 9).

Per Liden - I tried your suggestion of making jweak unconditionally tagged.  It doesn't eliminate much of the INCLUDE_ALL_GCS conditionals, since those are still needed to protect references to G1-specific code.  (The littering of otherwise generic runtime code with this G1-specific stuff should be a cleanup task for JDK 10.) However, it does simplify some of the code, and simplifies testing.
The cost is a "bit-test and (predicted taken) conditional branch" in the handling of an oop result from a native call.  I agree that's unlikely to be noticable, and the benefits seem worthwhile, so I've made that change.

I've attempted to update the platform-specific native call return handling changes for all of the platforms, including those I don't have access to (aarch64, ppc, s390).  I also fixed the zero change I'd made earlier.  And I made shark error, to force anyone using it to fix the handling of an oop result by SharkNativeWrapper::initialize().
There might be other zero/shark changes that I've missed.  All of these should be checked by appropriate platform experts.

I had to fix a JVMTI bug as part of this; FollowReferences wasn't doing as much argument checking as expected.  This was unconvered by a closed test that unexpectedly asserted.  This test would have similarly asserted previously if CheckJNICalls was true; I removed that conditionalization in JNIHandles::resolve, as it was only there to avoid an expensive check for the jobject being a jweak, and in the new code we already know it isn't.  There might be other JVMTI functions with similar issues.

Mikael - There are some ugly worms lurking under that rock!  I think I've dealt with the problem, tripping over some other bugs along the way.  Thanks for the test case.  I tweaked it a bit and included it in the updated changeset.

The passing of arguments involves collecting them into a JavaCallArguments object.  oop arguments are in some handlized form (either jobject or Handle).  The existing code heavily cheats; for example, it assumes one can correctly cast from a jobject to an oop* and dereference that pointer.  Obviously, a tagged jweak breaks that assumption.  Even worse, there are casts from oop* to oop and back!
(These are so nasty they *must* use C-style cast syntax.) I'm thinking about some followup cleanup bugs in that area.

To address this, I changed JavaCallArguments to avoid conversions from jobject to Handle/oop*.  To support that, it now keeps more accurate track of the state of the object references in the collected arguments.  It also delays resolving jobjects to their designated oops, avoiding intermediate Handle objects.  (This was a place where cheating was going on, in order to avoid intermediate Handle allocations.  We're now accomplishing that without crashing through the JNIHandles abstraction.)

CR:
https://bugs.openjdk.java.net/browse/JDK-8166188

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04
incr: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.inc
The full webrev may be more useful than the incremental; some of the incremental changes are pretty hard to read. 

Some additional webrevs that might be helpful:
http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.other-platforms
Incremental changes for aarch64, ppc, and s390 supplied by Andrew and Martin.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.always_tag[.full]
Incremental [full] changes to always tag jweaks.
The full webrev may be more useful than the incremental; some of the incremental changes are pretty hard to read. 

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04.java_call
Incremental changes to address the java call problem found by Mikael.

hotspot.04 == hotspot.02 + hotspot.04.inc
hotspot.04 == hotspot.04.always_tag.full + hotspot.04.java_call
hotspot.04 == hotspot.02 + hotspot.04.other-platforms +
              hotspot.04.always_tag + hotspot.04.java_call

The updated G1-specific test requiring the whitebox extension is
http://cr.openjdk.java.net/~kbarrett/8166188/test.04

The whitebox extension has not changed:
http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hsroot.01/
http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hotspot.01/


