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

Fri Feb 10 02:34:23 UTC 2017

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/sharedRuntime_x86_32.cpp.udiff.html
http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/sharedRuntime_x86_64.cpp.udiff.html
http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp.udiff.html

The code to unpack the oop result is the same at least for the x86 
implementations, which unfortunately still have 32/64 bit variants for 
sharedRuntime_x86.cpp.   So in the x86 case, there are 3.  I think these 
should be refactored into a function in macroAssembler_x86.cpp, 
something like unpack_native_oop_result(). It's not technically unboxing 
because boxing refers to making primitives into java.lang.Integer, etc 
objects.

Sorry that this might be more work and tricky for the other platforms 
because I didn't diff them, but it would be a lot better.   Maintaining 
separate copies of assembly code is a lot of work too.  You can file an 
RFE/bug to clean this up if there was not a reason for the duplication 
because of ZBB.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/prims/jni.cpp.udiff.html

Can you change 'l' to obj?  never mind, I see why you picked 'l' but it 
looks like a 1.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/runtime/javaCalls.hpp.udiff.html

I see what you've done.   These are terrible casts.

+ JNITypes::put_obj((oop)handle, _value, size); // Updates size.

I think as a future cleanup, we should make JNITypes::put_obj take 
(intptr_t) or (jobject,Handle) instead of dealing with the oop class for 
CheckUnhandledOops.  I didn't see any other callers except this one.

Why is value_state uint when it's being stored into uchar?

+ static const uint value_state_primitive = 0;

Could they be enums near where _value_state_buffer is declared?

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/runtime/jniHandles.hpp.udiff.html

I don't think it's good to have to include

+#include "gc/g1/g1SATBCardTableModRefBS.hpp"

in jniHandles.hpp header file.

could you refactor resolve_impl to put the jweak case in the .cpp file?  
I don't think performance is going to be important for jweak.  All this 
work for an indirection.  With the template code, it's hard to see 
that's all it is. :(

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/test/runtime/jni/ReturnJNIWeak/ReturnJNIWeak.java.html

Someone else asked this question but how can you assume that GC cleared 
the weak reference?

  115           System.gc();
  116           // Verify weak ref cleared as expected.

That's all.  The change looks good.

Thanks,
Coleen

On 2/6/17 8:03 PM, 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.
>
> 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/
>
>