Hi
On 6/9/20 12:34 PM, 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
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
