8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles (original) (raw)

Mikael Gerdin mikael.gerdin at oracle.com
Fri Feb 24 12:02:19 UTC 2017

Actually CC:ing Andrew.

On 2017-02-24 10:55, Mikael Gerdin wrote:

Hi Dan,

Thanks for looking at this review! On 2017-02-23 17:18, Daniel D. Daugherty wrote: On 2/23/17 6:29 AM, Mikael Gerdin wrote:

Hi Kim,

Thanks for the prompt review! On 2017-02-22 20:40, Kim Barrett wrote: On Feb 22, 2017, at 11:07 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:

Hi all, Please review this revised change to jweak to support G1. I've copied Kim's original description of the fix below for reference:

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::generatenativeentry and SharedRuntime::generatenativewrapper 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 g1writebarrierpre definitions from InterpreterMacroAssembler to MacroAssembler. Also moved g1writebarrierpost, for consistency. In addition to Kim's final version of the change (which was backed out) this updated version adds code to mask out the weak tag bit in the fast JNI field getters on all platforms where fast JNI getters are used. […] Webrevs: Kim's initial change: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/ My additions: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/ Full webrev: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/ Bugs: https://bugs.openjdk.java.net/browse/JDK-8175085 https://bugs.openjdk.java.net/browse/JDK-8166188 Thanks /Mikael Mikael - Thanks for taking this up for me. Just a few minor comments. ------------------------------------------------------------------------------ The usual copyright reminder. Updated copyrights. ------------------------------------------------------------------------------ src/cpu/arm/vm/jniFastGetFieldarm.cpp 122 #ifndef AARCH64 123 _ bic(R1, R1, JNIHandles::weaktagmask); 124 #else 125 _ andr(R1, R1, ~JNIHandles::weaktagmask); 126 #endif Usual style when selecting between AARCH64 and not seems to be to put the AARCH64 code first, e.g. #ifdef AARCH64 ... #else ... #endif Fixed. ------------------------------------------------------------------------------ src/cpu/x86/vm/jniFastGetFieldx8632.cpp 90 assert(invertedjweakmask == -2, "Otherwise check this code"); Use STATICASSERT rather than assert. ------------------------------------------------------------------------------ src/cpu/x86/vm/jniFastGetFieldx8632.cpp 89 const intptrt invertedjweakmask = ~staticcast(JNIHandles::weaktagmask); 90 assert(invertedjweakmask == -2, "Otherwise check this code"); 91 _ andl(rdx, invertedjweakmask); // mask is subject to sign-extension With three occurrences of this snippet in this file and two more similar ones in the 64-bit file, a macroAssembler helper seems called for here. ------------------------------------------------------------------------------ src/cpu/x86/vm/jniFastGetFieldx8664.cpp 84 const intptrt invertedjweakmask = ~staticcast(JNIHandles::weaktagmask); 85 const int32t truncatedmask = staticcast(invertedjweakmask); Why the two-step conversion here? Why not just const int32t truncatedmask = staticcast(JNIHandles::weaktagmask); I had changed this a bunch of times since I was a bit unsure about how to best do this but when I decided to do the bitwise invert after the cast to signed I forgot to clean up the casts. That would also make the 32 and 64-bit code more similar in the suggested macroAssembler helper. Indeed, I even found MacroAssembler::andptr which makes this a nice and small helper. ------------------------------------------------------------------------------ test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c 51 #define CHECK(variable, _expected) _ 52 do { if ((variable) != (expected)) _{ _ 53 (*env)->ThrowNew(env, exception, #variable" != " _#expected); _ _54 return; _ _55 } _ 56 } while(0); The formatting of the code in the macro body is strange and confusing. Cleaned up formatting. Also the do-while-zero idiom that I'm used to leaves off the final semicolon in the macro. Instead of the uses are expected to be terminated with semicolons, as you've done. Oh, oops. I also took the liberty to add STATICASSERTS on 64 bit arm since the encoding of the immediate in the "andr" instruction is a bit "funny". The assembler code only verifies that immediate encoding works in debug builds but in debug builds the fast JNI getters aren't generated. New webrevs: Incremental: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/ Full: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/ I only took a look at these files from the full webrev. These are the files touch in the two incremental webrevs: src/cpu/aarch64/vm/jniFastGetFieldaarch64.cpp No comments. src/cpu/arm/vm/jniFastGetFieldarm.cpp No comments. src/cpu/sparc/vm/jniFastGetFieldsparc.cpp No comments. src/cpu/x86/vm/jniFastGetFieldx8632.cpp No comments. src/cpu/x86/vm/jniFastGetFieldx8664.cpp No comments. src/cpu/x86/vm/macroAssemblerx86.cpp I like the new helper! src/cpu/x86/vm/macroAssemblerx86.hpp No comments. test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java L50 - any reason for the blank line? No reason whatsoever, I'll remove it. test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c L32 : JavaCallWithJNIWeaktestJNIFieldAccessors (JNIEnv *env, jclass clazz, jobject this) { L102: JavaCallWithJNIWeakrunTests (JNIEnv *env, jclass clazz, jobject this) { L137: JavaCallWithJNIWeakweakReceiverTest0 (JNIEnv *env, jobject obj) { Please delete the space before the first '('. Oh, oops. I'm going to blame javah for that :) You may want to add a comment between the two halves of the test for the literal values that you're setting in the .java file and testing for in the .c file. Added short comments in the .java and .c files. In your (Mikael G) original invite: Testing: Standard JPRT and tier2-3 HotSpot tests. Does tier2 and/or tier3 cover the tests that failed in Mach5? Somehow I forgot to mention that I also ran jdktier3 and the tests that failed in Mach5 did pass on all platforms. I've modified the CallWithJNIWeak test to call all the primitive getters and some other interesting cases I came up with. I've also run the CallWithJNIWeak test on all platforms on both product and fastdebug builds (since fast JNI getters are implicitly disabled in debug builds due to VerifyJNIFields being enabled by default in debug builds. Great coverage with the new tests! I've not built or tested the aarch64 port but I think it's correct and I hope someone can test it for me. Have you heard back from anyone on aarch64? I haven't seen any e-mail on this OpenJDK thread... I've cc:ed Andrew in this thread to see if I can catch his attention. New webrevs at: http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/ http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/ Thanks /Mikael Thumbs up! Dan Thanks /Mikael ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list