Hi
Thank you, I agree, it is safer to return.
Leonid
> On Jul 14, 2020, at 11:56 AM, Chris Plummer <chris.plum...@oracle.com> wrote:
>
> Hi Leonid,
>
> In allthr001.cpp I question if it is safe to proceed after the following
> error:
>
> 205 if (threadsCount - num_unexpected != thrInfo[ind].cnt + sys_cnt) {
> 206 printf("Point %d: number of threads expected: %d, got: %d\n",
> 207 ind, thrInfo[ind].cnt + sys_cnt, threadsCount -
> num_unexpected);
> 208 result = STATUS_FAILED;
> 209 }
>
> It seems it would be much safer to return on error, and even if not really
> necessary, there is not much point in continuing with the test code that
> follows.
>
> The rest of the changes look good. I don't need to see another webrev.
>
> thanks,
>
> Chris
>
> On 7/13/20 3:42 PM, Leonid Mesnik wrote:
>> Could I have 2nd reviewer?
>>
>> Leonid
>>
>>> On Jun 25, 2020, at 10:58 AM, Leonid Mesnik <leonid.mes...@oracle.com
>>> <mailto:leonid.mes...@oracle.com>> wrote:
>>>
>>> Ping
>>>
>>>> On Jun 12, 2020, at 4:18 PM, Leonid Mesnik <leonid.mes...@oracle.com
>>>> <mailto:leonid.mes...@oracle.com>> wrote:
>>>>
>>>> Fixed all places, updated copyright. Still need second review
>>>>
>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/
>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/>
>>>> Leonid
>>>>
>>>> On 6/11/20 8:41 PM, serguei.spit...@oracle.com
>>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>>> Hi Leonid,
>>>>>
>>>>> It is much better now.
>>>>>
>>>>> Several places still need the same fix.
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html>
>>>>>
>>>>> 211 for (i = 0; i < thrInfo[ind].cnt; i++) {
>>>>> 212 for (j = 0, found = 0; j < threadsCount && !found; j++) {
>>>>> 213 err = jvmti->GetThreadInfo(threads[j], &inf);
>>>>> 214 if (err != JVMTI_ERROR_NONE) {
>>>>> 215 printf("Failed to get thread info: %s (%d)\n",
>>>>> 216 TranslateError(err), err);
>>>>> 217 result = STATUS_FAILED;
>>>>> 218 }
>>>>> 219 if (printdump == JNI_TRUE) {
>>>>> 220 printf(" >>> %s", inf.name);
>>>>> 221 }
>>>>> 222 found = (inf.name != NULL &&
>>>>> 223 strstr(inf.name, thrInfo[ind].thrNames[i]) ==
>>>>> inf.name &&
>>>>> 224 (ind == 4 || strlen(inf.name) ==
>>>>> 225 strlen(thrInfo[ind].thrNames[i])));
>>>>> 226 }
>>>>> A return is needed after line 217, otherwise the the inf value is used at
>>>>> lines 222-224.
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html>
>>>>>
>>>>> A return is needed for the errors:
>>>>> 363 result = STATUS_FAILED;
>>>>> 372 result = STATUS_FAILED;
>>>>> 384 result = STATUS_FAILED;
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html>
>>>>>
>>>>> A return is needed for the errors:
>>>>> 82 result = STATUS_FAILED;
>>>>> 94 result = STATUS_FAILED;
>>>>> 100 result = STATUS_FAILED;
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html>
>>>>>
>>>>> A return is needed for the error:
>>>>> 98 result = STATUS_FAILED;
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html>
>>>>>
>>>>> A return is needed for the error:
>>>>> 98 result = STATUS_FAILED;
>>>>>
>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html
>>>>>
>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html>
>>>>>
>>>>> A return is needed for the error:
>>>>> 186 result = STATUS_FAILED;
>>>>>
>>>>> Also, I do not like many uninitialized locals in these tests.
>>>>> But it is for another pass.
>>>>>
>>>>> Otherwise, it looks good.
>>>>> No need for another webrev if you fix the above.
>>>>> I hope, you will update copyright comments before push.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 6/11/20 15:30, Leonid Mesnik wrote:
>>>>>> Agree, it would be better to don't try to use data from functions with
>>>>>> error code. The new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/
>>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/>
>>>>>> I tried to prevent any usage of possibly corrupted data. Mostly strings
>>>>>> or allocated data, sometimes method/class id which are used my other
>>>>>> JVMTI functions.
>>>>>>
>>>>>> Leonid
>>>>>>
>>>>>> On 6/9/20 6:59 PM, serguei.spit...@oracle.com
>>>>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>>>>> On 6/9/20 12:58, Leonid Mesnik wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/9/20 12:34 PM, serguei.spit...@oracle.com
>>>>>>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>>>>>>> Hi Leonid,
>>>>>>>>>
>>>>>>>>> Thank you for taking care about this!
>>>>>>>>> It looks good in general.
>>>>>>>>> However, I think, a similar return is needed in more cases.
>>>>>>>>>
>>>>>>>>> One example:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html
>>>>>>>>>
>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html>
>>>>>>>>>
>>>>>>>>> 99 err = jvmti_env->GetMethodDeclaringClass(method, &cls);
>>>>>>>>> 100 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 101 printf("(GetMethodDeclaringClass#t) unexpected error: %s
>>>>>>>>> (%d)\n",
>>>>>>>>> 102 TranslateError(err), err);
>>>>>>>>> 103 result = STATUS_FAILED;
>>>>>>>>> <> 104 return;
>>>>>>>>> 105 }
>>>>>>>>> 106 err = jvmti_env->GetClassSignature(cls, &ex.t_cls, &generic);
>>>>>>>>> 107 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 108 printf("(GetClassSignature#t) unexpected error: %s
>>>>>>>>> (%d)\n",
>>>>>>>>> 109 TranslateError(err), err);
>>>>>>>>> 110 result = STATUS_FAILED;
>>>>>>>>> 111 }
>>>>>>>>> 112 err = jvmti_env->GetMethodName(method,
>>>>>>>>> 113 &ex.t_name, &ex.t_sig, &generic);
>>>>>>>>> 114 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 115 printf("(GetMethodName#t) unexpected error: %s (%d)\n",
>>>>>>>>> 116 TranslateError(err), err);
>>>>>>>>> 117 result = STATUS_FAILED;
>>>>>>>>> 118 }
>>>>>>>>> 119 ex.t_loc = location;
>>>>>>>>> 120 err = jvmti_env->GetMethodDeclaringClass(catch_method, &cls);
>>>>>>>>> 121 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 122 printf("(GetMethodDeclaringClass#c) unexpected error: %s
>>>>>>>>> (%d)\n",
>>>>>>>>> 123 TranslateError(err), err);
>>>>>>>>> 124 result = STATUS_FAILED;
>>>>>>>>> <> 125 return;
>>>>>>>>> 126 }
>>>>>>>>> 127 err = jvmti_env->GetClassSignature(cls, &ex.c_cls, &generic);
>>>>>>>>> 128 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 129 printf("(GetClassSignature#c) unexpected error: %s
>>>>>>>>> (%d)\n",
>>>>>>>>> 130 TranslateError(err), err);
>>>>>>>>> 131 result = STATUS_FAILED;
>>>>>>>>> 132 }
>>>>>>>>> 133 err = jvmti_env->GetMethodName(catch_method,
>>>>>>>>> 134 &ex.c_name, &ex.c_sig, &generic);
>>>>>>>>> 135 if (err != JVMTI_ERROR_NONE) {
>>>>>>>>> 136 printf("(GetMethodName#c) unexpected error: %s (%d)\n",
>>>>>>>>> 137 TranslateError(err), err);
>>>>>>>>> 138 result = STATUS_FAILED;
>>>>>>>>> 139 }
>>>>>>>>>
>>>>>>>>> In the fragment above you added return for JVMTI
>>>>>>>>> GetMethodDeclaringClass error.
>>>>>>>>> But GetMethodName and GetClassSignature can be also problematic as
>>>>>>>>> the returned names are printed below.
>>>>>>>>> It seems to be more safe and even simpler to add returns for such
>>>>>>>>> cases as well.
>>>>>>>>> Otherwise, the code reader is puzzled why there is a return in one
>>>>>>>>> failure case and there is no such return in another.
>>>>>>>> It is a good question if we want to fix such places or even fails with
>>>>>>>> first JVMTI failure. (I even started to fix it in the such way but
>>>>>>>> find that existing tests usually don't fail always).
>>>>>>>>
>>>>>>>
>>>>>>> I do not suggest to fix all the tests but those which you are already
>>>>>>> fixing.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> The difference is that test tries to reuse "cls" in other JVMTI
>>>>>>>> function and going to generate very misleading crash. How it just
>>>>>>>> tries to compare ex and exs values. So test might crash but clearly
>>>>>>>> outside of JVMTI function and with some useful info. So I am not sure
>>>>>>>> if fixing these lines improve test failure handling.
>>>>>>>>
>>>>>>>
>>>>>>> If JVMTI functions fail with an error code the results with symbolic
>>>>>>> strings must be considered invalid.
>>>>>>> However, they are used later (the values are printed).
>>>>>>> It is better to bail out in such cases.
>>>>>>> It should not be a problem to add similar returns in such cases.
>>>>>>> Or do you think it is important to continue execution for some reason?
>>>>>>>
>>>>>>>> Assuming that most of existing tests fails early only if going to
>>>>>>>> re-use possible corrupted data I propose to fix this separately. We
>>>>>>>> need to figure out when to fail or to try to finish.
>>>>>>>>
>>>>>>>
>>>>>>> Do you suggest it for the updated tests only or for all the tests with
>>>>>>> such problems?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/1/20 21:33, Leonid Mesnik wrote:
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> Could you please review following fix which stop test execution if
>>>>>>>>>> JVMTI function returns error. The test fails anyway however using
>>>>>>>>>> potentially bad data in JVMTI function might cause misleading crash
>>>>>>>>>> failures. The hs_err will contains the stacktrace not with problem
>>>>>>>>>> function but with function called with corrupted data. Most of tests
>>>>>>>>>> already has such behavior but not all. Also I fixed a couple of
>>>>>>>>>> tests to finish if they haven't managed to suspend thread.
>>>>>>>>>>
>>>>>>>>>> I've updated only tests which try to use corrupted data in JVMTI
>>>>>>>>>> functions after errors. I haven't updated tests which just
>>>>>>>>>> compare/print values from erroring JVMTI functions. The crash in
>>>>>>>>>> strcmp/println is not so misleading and might be point to real
>>>>>>>>>> issue.
>>>>>>>>>>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/
>>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8242891
>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8242891>
>>>>>>>>>>
>>>>>>>>>> Leonid
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>
