JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

[David Holmes]

On 7/12/2017 2:55 PM, Ian Rogers wrote:
> Thanks, fwiw Jikes RVM agrees that all critical releases end the 
> critical region:
> https://github.com/JikesRVM/JikesRVM/blob/master/rvm/src/org/jikesrvm/jni/JNIFunctions.java#L5936
> 
> As does Cacao:
> http://mips.complang.tuwien.ac.at/hg/cacao/file/2b4c9b6c245d/src/native/jni.cpp#l3208
> 
> Kaffe (with a non-moving GC) treats the release in the same way as 
> Release<Primitive>ArrayElements:
> https://github.com/kaffe/kaffe/blob/master/kaffe/kaffevm/jni/jni-arrays.c#L404
> which is the same as how it is handled on Android.
> 
> HotSpot has managed to put a foot in both camps, ending the critical 
> always (a la Jikes RVM and Cacao) but honoring commit behavior (and 
> leaking) when in -Xcheck:jni.

So ... I should file a bug for the leak in the checked version and apply 
your suggested patch to always pass mode 0?

Thanks,
David


> Thanks,
> Ian
> 
> On Wed, Dec 6, 2017 at 5:25 PM, David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     Hi Ian,
> 
>     On 7/12/2017 4:48 AM, Ian Rogers wrote:
> 
>         Thanks David, I think we may have differing opinions on clarity
>         :-) In the case of -Xcheck:jni and with HotSpot a copy is always
>         made, and so
> 
> 
>     Ah I see. I was considering only the "real" functionality.
> 
>     The fact the checked version of getPrimitiveArrayCritical creates a
>     copy seems to be treated as an internal detail as it does not AFAICS
>     report that a copy was in fact made - the passed in isCopy pointer
>     will be set false by the real getPrimitiveArrayCritical.
> 
>         perhaps the attached patch causes the implementation to match
>         the spec.
> 
> 
>     The whole logic surrounding the management of the copy seems
>     somewhat suspect to me. As you note with your patch
>     check_wrapped_array_release honours the mode, which in the checked
>     case allows, AFAICS, the internally made copy to "leak". So ignoring
>     the passed in mode and always passing 0 to
>     check_wrapped_array_release seems more correct.
> 
>         Given that commit doesn't free a copy, is it intended behavior
>         that in HotSpot it ends a critical region? ie In a VM that
>         allowed copies you could imagine, Get, Commit, Release and as
>         spec-ed the critical region ends at the commit if not a copy,
>         and at the release if it is. Perhaps the simplest thing is to
>         remove the notion of commit here.
> 
> 
>     I honestly do not know what this API is really trying to achieve in
>     that regard. I don't know the intended usage of "commit" mode. Why
>     would you not free the copied buffer? The spec unhelpfully only states:
> 
>     "The other options give the programmer more control over memory
>     management and should be used with extreme care."
> 
>     And if you don't free it what can you do with it? For
>     Get*ArrayElements it clearly states
> 
>     "The result is valid until the corresponding
>     Release<PrimitiveType>ArrayElements() function is called."
> 
>     so once any Release* is done, regardless of mode, it would seem the
>     array (copy or not) is no longer valid and should not be accessed
>     again. So I don't see any expectation that you can call Release*
>     multiple times and so in terms of the critical variants I expect the
>     critical section to end when Release is called - regardless of mode.
> 
>     David
> 
>         Thanks,
>         Ian
> 
> 
>         On Tue, Dec 5, 2017 at 4:37 PM, David Holmes
>         <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>         <mailto:david.holmes at oracle.com
>         <mailto:david.holmes at oracle.com>>> wrote:
> 
>              Hi Ian,
> 
>              On 6/12/2017 9:15 AM, Ian Rogers wrote:
> 
>                  From:
>         https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical>
>                 
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical>>
> 
> 
>              You need to see the updated spec:
> 
>         https://docs.oracle.com/javase/9/docs/specs/jni/functions.html#getprimitivearraycritical-releaseprimitivearraycritical
>         <https://docs.oracle.com/javase/9/docs/specs/jni/functions.html#getprimitivearraycritical-releaseprimitivearraycritical>
>             
>         <https://docs.oracle.com/javase/9/docs/specs/jni/functions.html#getprimitivearraycritical-releaseprimitivearraycritical
>         <https://docs.oracle.com/javase/9/docs/specs/jni/functions.html#getprimitivearraycritical-releaseprimitivearraycritical>>
> 
>              That spec makes it clear:
> 
>              "mode: the release mode (see Primitive Array Release Modes): 0,
>              JNI_COMMIT or JNI_ABORT. Ignored if carray was a not copy."
> 
>              In hotspot getCriticalArrayPrimitive never returns a copy
>         so the
>              mode is always ignored, as per the existing comment.
> 
>              David
>              -----
> 
> 
>                  "The semantics of these two functions are very similar
>         to the
>                  existing
>                  Get/Release<primitivetype>ArrayElements functions. "
> 
>         https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#Release_PrimitiveType_ArrayElements_routines
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#Release_PrimitiveType_ArrayElements_routines>
>                 
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#Release_PrimitiveType_ArrayElements_routines
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#Release_PrimitiveType_ArrayElements_routines>>
> 
>                  "JNI_COMMIT copy back the content but do not free the
>         elems buffer"
> 
>                  Consider the pattern of:
>                  GetPrimitiveArrayCritical(...)
>                  ReleasePrimitiveArrayCritical(..., JNI_COMMIT)
>                  ReleasePrimitiveArrayCritical(..., 0)
> 
>                  where the first release is to just achieve a copy back.
>         For example,
>                  jniCheck.cpp will copy back but not free a copy in the
>         case of
>                  JNI_COMMIT
>                  for a critical. The implementation of
>                  ReleasePrimitiveArrayCritical ignores
>                  all arguments and so it ends the critical region even
>         in the
>                  event of a
>                  commit.
> 
>                  The attached patch makes ReleasePrimitiveArrayCritical
>         consider
>                  the mode
>                  argument when releasing the GCLocker.
> 
>                  Thanks,
>                  Ian
> 
> 
>