8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

[Kim Barrett]

> 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::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.
>> 
> 
> 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.

------------------------------------------------------------------------------  
src/cpu/arm/vm/jniFastGetField_arm.cpp
 122 #ifndef AARCH64
 123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
 124 #else
 125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
 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

------------------------------------------------------------------------------
src/cpu/x86/vm/jniFastGetField_x86_32.cpp 
  90   assert(inverted_jweak_mask == -2, "Otherwise check this code");

Use STATIC_ASSERT rather than assert.

------------------------------------------------------------------------------ 
src/cpu/x86/vm/jniFastGetField_x86_32.cpp 
  89   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
  90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
  91   __ andl(rdx, inverted_jweak_mask); // 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/jniFastGetField_x86_64.cpp 
  84   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
  85   const int32_t truncated_mask = static_cast<int32_t>(inverted_jweak_mask);

Why the two-step conversion here?  Why not just

  const int32_t truncated_mask = static_cast<int32_t>(JNIHandles::weak_tag_mask);

That would also make the 32 and 64-bit code more similar in the
suggested macroAssembler 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.

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.

------------------------------------------------------------------------------