RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return error

[Leonid Mesnik]

Hi


On 6/9/20 12:34 PM, serguei.spitsyn at 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
>
>   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).

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.

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.

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/
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8242891
>>
>> Leonid
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200609/922af478/attachment-0001.htm>