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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Feb 8 22:13:20 UTC 2017 

 > New webrevs:
 > full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04

make/test/JtregNative.gmk
     No comments.

src/share/vm/runtime/javaCalls.hpp
     No comments.

src/share/vm/runtime/javaCalls.cpp
     L526:                 "signature does not matched pushed arguments: 
%u", state);
         Typo: 'matched' -> 'match'

     For the guarantees on L522 and L525, would the (_pos - 1) value
     be useful for diagnostic purposes?

     L569:       if (v != 0 ) {
         Not your bug, but there's an extra blank before the ")".

     L571:         if (t < (size_t)os::vm_page_size()) {
         Again, not your issue, but that size < page size check means
         the oop is bad is screaming for a short comment.

src/share/vm/runtime/jniHandles.hpp
     (old) L208:     *((oop*)handle) = deleted_handle(); // Mark the 
handle as deleted, allocate will reuse it
         The old comment was too obvious so you didn't keep it? :-)

     It threw me for a second when JNIHandles::resolve_non_null()
     was switched to use resolve_impl() instead of guard_value(),
     then I figured out that resolve_impl() uses guard_value().

src/share/vm/runtime/jniHandles.cpp
     (old) L115     assert(!CheckJNICalls || 
is_weak_global_handle(handle), "Invalid delete of weak global JNI handle");

         Don't like the old assert? Is is_weak_global_handle() now obsolete?

src/share/vm/prims/jni.cpp
     No comments.

src/share/vm/prims/jvmtiEnv.cpp
     Need a copyright year update in this file.


src/cpu/aarch64/vm/sharedRuntime_aarch64.cpp
     No comments.

src/cpu/aarch64/vm/templateInterpreterGenerator_aarch64.cpp
     No comments.

src/cpu/arm/vm/interp_masm_arm.cpp
     No comments on the code deletion.

src/cpu/arm/vm/interp_masm_arm.hpp
     No comments on the code deletion.

src/cpu/arm/vm/macroAssembler_arm.cpp
     OK, the code from interp_masm_arm.cpp moved here.
     Did not try to compare the two versions.

src/cpu/arm/vm/macroAssembler_arm.hpp
     OK, the code from interp_masm_arm.hpp moved here.
     Did not try to compare the two versions.

src/cpu/arm/vm/sharedRuntime_arm.cpp
     No comments.

src/cpu/arm/vm/templateInterpreterGenerator_arm.cpp
     No comments.

src/cpu/ppc/vm/frame_ppc.cpp
     No comments.

src/cpu/ppc/vm/sharedRuntime_ppc.cpp
     No comments.

src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp
     No comments.

src/cpu/s390/vm/sharedRuntime_s390.cpp
     No comments.

src/cpu/s390/vm/templateInterpreterGenerator_s390.cpp
     (old) L1705:   __ verify_oop(Rlresult);
         The delete of the verify_oop() call after the store_oop_result
         label threw me for a few minutes. Then I realized that the
         only uses of the store_oop_result label appear to be when we
         have a NULL oop. New code is cleaner.

src/cpu/sparc/vm/sharedRuntime_sparc.cpp
     No comments.

src/cpu/sparc/vm/templateInterpreterGenerator_sparc.cpp
     Not your bug, but unlike sharedRuntime_sparc.cpp, there are no
     verify_oop calls here.

src/cpu/x86/vm/sharedRuntime_x86_32.cpp
     No comments.

src/cpu/x86/vm/sharedRuntime_x86_64.cpp
     No comments.

src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp
     Not your bug, but unlike sharedRuntime_x86{32,64}.cpp,
     there are no verify_oop calls here.

src/cpu/zero/vm/cppInterpreter_zero.cpp
     No comments.

src/share/vm/shark/sharkNativeWrapper.cpp
     No comments.

test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
     jcheck will likely complain about the empty line at the end
     of the file.

test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
     Same empty line at the end of the file comment.

test/runtime/jni/ReturnJNIWeak/ReturnJNIWeak.java
     No comments.

test/runtime/jni/ReturnJNIWeak/libReturnJNIWeak.c
     Nit - libCallWithJNIWeak.c uses an indent of 4 spaces;
     this file uses an indent of 2 spaces. Should probably
     pick one...


Thumbs up! If you choose to fix any of the nits above, I don't
need to see another webrev.

Dan


On 2/6/17 6: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/
>
>
>

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