Re: RFR (M) 8223044: Add back exception checking in tests

Mon, 06 May 2019 19:10:48 -0700

+1

On 5/6/19 6:16 PM, [email protected] wrote:
Hi Jc,

It looks great to me.
Thank you for the update!

Thanks,
Serguei


On 5/6/19 17:37, Jean Christophe Beyler wrote:
Hi both,

I think it all makes sense so I did this:

@Serguei: I think since I am changing the call sites anyway, it makes sense to just change it to ec_jni for all; so I went ahead and did it retro-actively to all call sites
@Chris: I am not shocked or my eyes are not crying by seeing TRACE_JNI_CALL_VARARGS  so I went ahead and did that as well; I think it works out better too:

You both said "leave it up to you" but I'd rather wait for a final LGTM from both (and/or reviews from others) of you and I can move forward, I'll re-submit for testing via the submit repo (all the affected tests pass on my dev machine already).

Thanks again,
Jc

On Mon, May 6, 2019 at 11:32 AM Chris Plummer <[email protected]> wrote:
Hi JC,

After looking at the following:

  71     methodID = jni_env->GetMethodID(
  72             klass, "loadClass", "(Ljava/lang/String;)Ljava/lang/Class;", TRACE_JNI_CALL);
  73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACE_VARARGS(className));

I was wondering if the naming of TRACE_VARARGS should more closely resemble TRACE_JNI_CALL. Maybe TRACE_JNI_VARARGS or TRACE_JNI_CALL_VARARGS (starting to get a bit wordy though)?

I'll leave it up to you. Other than that it looks fine.

thanks,

Chris

On 5/4/19 2:12 PM, Jean Christophe Beyler wrote:
Hi Chris and Serguei,

Then the new webrev is here:
Let me know if now it looks good for you;
@Serguei, you had said it looks good but had comments; you never answered my comment about naming; would you like me to rename all the environments to ec_jni_env? I'd do another webrev for the ones not done in this webrev but that are already under the ExceptionCheckingJniEnvPtr, let me know :)

Finally, does anyone else have comments?

Thanks,
Jc


On Tue, Apr 30, 2019 at 3:46 PM Chris Plummer <[email protected]> wrote:
On 4/30/19 3:36 PM, Jean Christophe Beyler wrote:
You are right I was overthinking the naming, so I did this now:

-    loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACE_JNI_CALL, className);
+    loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACE_VARARGS(className));
Ok. I like this.

Question now is: this variadic won't work if there are no arguments in a portable way, so if there are no arguments for the call, should we use a TRACE_JNI_CALL such as:
    jni_env->CallVoidMethod(thread, methodID, TRACE_JNI_CALL);

or should I create a TRACE_VARARGS for no arguments:
    jni_env->CallVoidMethod(thread, methodID, TRACE_EMPTY_VARARGS());

Advantage of the latter is that you would know it is a VARARG at least and you can see it; draw-back is yet another macro.

What do you think?
In general we don't know it is vararg when looking at a callsite, so I don't feel the need to advertise it as such with TRACE_EMPTY_VARARGS(). I'd recommend just directly passing TRACE_JNI_CALL.

Chris
Jc



On Tue, Apr 30, 2019 at 11:37 AM Chris Plummer <[email protected]> wrote:
Hi JC,

I'm not sure why you are suggesting TRACED_JNI_CALL instead of TRACED_VARARGS. How are they different? Are you suggesting non-varargs calls would just end up using an empty TRACED_JNI_CALL()?

Chris

On 4/30/19 10:05 AM, Jean Christophe Beyler wrote:
I do have more of these coming and there are 86 CallXMethod cases where this will occur and 49 NewObject cases. So it would be good to figure out something that does not hurt our eyes too many times.

I think I would go with the TRACED_VARARGS, it at least has the advantage of not being too bad. I would move toward doing:
 73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACED_JNI_CALL(className));

To be consisted with the non-vararg cases, what do you think?
Jc

On Tue, Apr 30, 2019 at 9:51 AM Chris Plummer <[email protected]> wrote:
The advantage of the TRACED_VARARGS approach (or leaving it as-is) is that there are limited APIs and callsites affected (only 1 of each in this review, but it's unclear to me if you have more of these changes coming).

Chris

On 4/29/19 9:49 PM, Jean Christophe Beyler wrote:
Yes I would fix them all in the next webrev and then continue the porting. I 100% agree to which is less offensive. I find that

 73     loadedClass = (jclass) jni_env->CALLOBJECTMETHODTRACED(loader, methodID, className);

to be offensive as well.

So perhaps the TRACED_VARARGS is best:


 73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACED_VARARGS(className));

What do you think?

On Mon, Apr 29, 2019 at 9:44 PM Chris Plummer <[email protected]> wrote:
Hi JC,

On 4/29/19 4:25 PM, Jean Christophe Beyler wrote:
Hi Chris,

I agree it's not ideal, how about putting it first?

  73     loadedClass = (jclass) jni_env->CallObjectMethod(TRACE_JNI_CALL, loader, methodID, className);
It's not consistent with other uses, or are you suggesting changing them all?

I find that less awkward than the VAR_ARGS, no?

Or something like:
 73     loadedClass = (jclass) jni_env->CallObjectMethodTraced(loader, methodID, className);

Where CallObjectMethodTraced is actually a define in the exception handling system that adds the line and filename...
I don't like macroizing a method call in this manner (macros should be all upper case, right?). Also it's inconsistent with the other tracing calls, unless you plan on doing it to all of them.

Which looks better?

Not sure. I wouldn't ask which is better. I'd ask which is less offensive. :)

thanks,

Chris


Thanks again,
Jc

On Mon, Apr 29, 2019 at 3:40 PM Chris Plummer <[email protected]> wrote:
Ok. I only half heatedly suggest the following

  73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, VAR_ARGS(className));

And then VAR_ARGS can contain the reference to TRACE_JNI_CALL.

Chris

On 4/29/19 2:58 PM, Jean Christophe Beyler wrote:
Hi Chris,

Thanks for the review! It is the only issue I have with the system now and I had not found a good solution for:

as CallObjectMethod is a variadic method, I can't put TRACE_JNI_CALL at the end; so I put right before the end; that allows me to do this:

+jobject ExceptionCheckingJniEnv::CallObjectMethod(jobject obj, jmethodID methodID, int line,
+                         const char* file_name, ...) {
+  JNIVerifier<> marker(this, "CallObjectMethod", obj, methodID, line, file_name);
+
+  va_list args;
+  va_start(args, file_name);
+  jobject result = _jni_env->CallObjectMethodV(obj, methodID, args);
+  va_end(args);
+  return result;
+}
Putting it at the end would mean I would have to play with the va_list to remove the last one, and from what I have understood with va_list, it's kind of a no-no to do so. So I left at this.
Do you have any better suggestion?

Thanks!
Jc

On Mon, Apr 29, 2019 at 10:38 AM Chris Plummer <[email protected]> wrote:
Hi JC,

In em01t002.cpp, is this correct?

  73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID, TRACE_JNI_CALL, className);

Shouldn't the TRACE_JNI_CALL arg be last? If so, can you look into why this test didn't fail as a result.

Other than that the changes look good.

thanks,

Chris

On 4/26/19 5:52 PM, Jean Christophe Beyler wrote:
Hi all,

(Re-sending with subject line complete :-))

Since JDK-8213501 finally merged (sorry it took so long), I am able to continue this work. Here is the work that puts back the messages for any calls that were moved around due to JDK-8212884.


All modified tests pass on my local dev machine.

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc




--

Thanks,
Jc

Reply via email to