Ping
> On Jun 12, 2020, at 4:18 PM, Leonid Mesnik <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
>>>>>>>
>>>>>>
>>>>
>>
