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