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

Jean Christophe Beyler jcbeyler at google.com
Tue Apr 30 22:36:28 UTC 2019


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));

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?
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190430/60d4a275/attachment-0001.html>


More information about the serviceability-dev mailing list