RFR (M) 8223044: Add back exception checking in tests
Jean Christophe Beyler
jcbeyler at google.com
Tue May 7 00:37:01 UTC 2019
Hi both,
I think it all makes sense so I did this:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.02/
Incremental webrev:
http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.01_02/
@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:
http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/events/EM01/em01t002/em01t002.cpp.udiff.html
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 <chris.plummer at oracle.com>
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:
> 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
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190506/9c7e9e78/attachment-0001.html>
More information about the serviceability-dev
mailing list