RFR (M) 8223044: Add back exception checking in tests
Jean Christophe Beyler
jcbeyler at google.com
Tue May 7 14:44:22 UTC 2019
Thanks both for your help :)
Jc
On Mon, May 6, 2019 at 7:09 PM Chris Plummer <chris.plummer at oracle.com>
wrote:
> +1
>
> On 5/6/19 6:16 PM, serguei.spitsyn at oracle.com 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:
> 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
>
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190507/98183b07/attachment-0001.html>
More information about the serviceability-dev
mailing list