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

Sat May 4 21:12:27 UTC 2019

Hi Chris and Serguei,

Then the new webrev is here:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8223044

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 <chris.plummer at oracle.com>
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 <chris.plummer at oracle.com>
> 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 <chris.plummer at oracle.com>
>> 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 <chris.plummer at oracle.com>
>>> 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 <chris.plummer at oracle.com>
>>>> 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 <
>>>>> chris.plummer at oracle.com> 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.
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8223044
>>>>>>
>>>>>> All modified tests pass on my local dev machine.
>>>>>>
>>>>>> Thanks,
>>>>>> Jc
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Thanks,
>>>>> Jc
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>>>
>>>
>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190504/933a828d/attachment-0001.html>