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

Reply via email to