Re: strict jni checking

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:

Mark> This implements a couple of ideas from bug #19512 (Optional JNI error
Mark> checking). When enabled by setting the system property
Mark> gnu.gcj.jni=strict you will get output like this when some strict JNI
Mark> constraint fails:

Nice.

Mark> We could probably return a trampoline
Mark> when strict_jni is enabled that extracts and registers the method
Mark> arguments before calling into the actual function. Volunteers welcome!

Yeah, I think this would be a reasonable approach.  It actually isn't
hard to implement; both the interpreter and GDirect make trampolines
like this already.

Mark> I ran various programs with and without strict checking enabled to check
Mark> that it works like intended. Enabling checking makes some JNI methods a
Mark> little slower, but not really noticeable.

I'd like some more profiling... do you have oprofile or something like
that installed?  I'm pretty sure the overhead will be minimal, but it
would be better to be sure.

Mark> +  if (strict_jni)
Mark> +    fprintf(stderr, "Enabling strict (slow) JNI checks\n");

I'd rather we not print anything.

Mark> +  // Make sure the reference is from either the local or global
Mark> +  // reference pool.
Mark> +  // XXX - We cannot currently enable this check since we don't
Mark> +  // register arguments to the native jni methods.
Mark> +#if 0
Mark> +  if (__builtin_expect (strict_jni, false) && obj != NULL)
Mark> +    {
Mark> +      bool local_ref_found = false;
Mark> +      _Jv_JNI_LocalFrame *frame;
Mark> +      JNIEnv *env = _Jv_GetCurrentJNIEnv ();
Mark> +      for (frame = env->locals; frame != NULL; frame = frame->next)
Mark> +	for (int i = 0; i < frame->size; ++i)
Mark> +	  if (frame->vec[i] == obj)
Mark> +	    local_ref_found = true;
Mark> +      
Mark> +      JvJNIAssert (local_ref_found || global_ref_table->containsKey (obj));
Mark> +    }
Mark> +#endif

Use "FIXME" instead of "XXX".
Ordinarily we don't have '#if 0' code, but in this case I think it is
fine to leave it in.

Mark> -  for (frame = env->locals; frame != NULL; frame = frame->next)
Mark> +  // Deletes the reference only from the top frame
Mark> +  // (if it is in that frame, otherwise this operation does nothing).
Mark> +  _Jv_JNI_LocalFrame *frame = env->locals;

When you say "the top frame", does that include when we auto-expand
and add a new frame?  Surely it should be anywhere in the expanded
pool?

Also, shouldn't there be a JvJNIAssert if no local reference is
found?  Or is deleting a non-existent local reference ok?
(Or did I misread the patch?  Occasionally unidiffs can be confusing.)

Mark> +  java::util::Properties *props = java::lang::System::properties;
Mark> +  if (props->get (JvNewStringLatin1 ("gnu.gcj.jni")))
Mark> +    _Jv_JNI_Init (true);
Mark> +  else
Mark> +    _Jv_JNI_Init (false);

Do we want to enforce some specific value of the property here?

I think perhaps the property should be named "gnu.gcj.jni.strict" or
something like that.  Just "jni" is too plain.

Also, please document the property in gcc/java/gcj.texi.


This is looking good.  I'm sure the Sable guys will be happy once we
are all writing error-free JNI all the time :-)

Tom