[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return
From: Leonid Mesnik <leonid.mesnik () oracle ! com>
Date: 2020-06-25 17:58:50
Message-ID: 611D0AF5-1295-4DD4-B6C9-F4D379AA069C () oracle ! com
[Download RAW message or body]
Ping
> On Jun 12, 2020, at 4:18 PM, Leonid Mesnik <leonid.mesnik@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.spitsyn@oracle.com <mailto:serguei.spitsyn@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.spitsyn@oracle.com \
> > > <mailto:serguei.spitsyn@oracle.com> wrote:
> > > > On 6/9/20 12:58, Leonid Mesnik wrote:
> > > > > Hi
> > > > >
> > > > >
> > > > > On 6/9/20 12:34 PM, serguei.spitsyn@oracle.com \
> > > > > <mailto:serguei.spitsyn@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
> > > > > > >
> > > > > >
> > > >
> >
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; line-break: after-white-space;" class="">Ping<br class=""><div><br \
class=""><blockquote type="cite" class=""><div class="">On Jun 12, 2020, at 4:18 PM, \
Leonid Mesnik <<a href="mailto:leonid.mesnik@oracle.com" \
class="">leonid.mesnik@oracle.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class="">
<div class=""><p class="">Fixed all places, updated copyright. Still need second \
review</p><p class=""><a \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/" \
class="">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.02/</a></p><p \
class="">Leonid<br class=""> </p>
<div class="moz-cite-prefix">On 6/11/20 8:41 PM,
<a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br \
class=""> </div>
<blockquote type="cite" \
cite="mid:3927ae7c-efa9-eb9f-ab98-18d778d5a966@oracle.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class="">
<div class="moz-cite-prefix">Hi Leonid,<br class="">
<br class="">
It is much better now.<br class="">
<br class="">
Several places still need the same fix.<br class="">
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html</a><br \
class=""> <br class="">
<pre class=""> 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 }</pre>
A return is needed after line 217, otherwise the the inf value
is used at lines 222-224.<br class="">
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html</a><br \
class=""> <br class="">
A return is needed for the errors:<br class="">
<pre class=""> 363 result = STATUS_FAILED;
372 result = STATUS_FAILED;
384 result = STATUS_FAILED;</pre>
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html</a><br \
class=""> <br class="">
A return is needed for the errors:<br class="">
<pre class=""> 82 result = STATUS_FAILED;
94 result = STATUS_FAILED;
100 result = STATUS_FAILED;</pre>
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html</a><br \
class=""> <br class="">
A return is needed for the error:<br class="">
<pre class=""> 98 result = STATUS_FAILED;</pre>
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html</a><br \
class=""> <br class="">
A return is needed for the error:<br class="">
<pre class=""> 98 result = STATUS_FAILED;</pre>
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html</a><br \
class=""> <br class="">
A return is needed for the error:<br class="">
<pre class=""> 186 result = STATUS_FAILED;</pre>
<br class="">
Also, I do not like many uninitialized locals in these tests.<br class="">
But it is for another pass.<br class="">
<br class="">
Otherwise, it looks good.<br class="">
No need for another webrev if you fix the above.<br class="">
I hope, you will update copyright comments before push.<br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 6/11/20 15:30, Leonid Mesnik wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:2cf4e45a-4d44-3c0a-a272-480f56a5e6e8@oracle.com" class=""><p \
class="">Agree, it would be better to don't try to use data from functions with \
error code. The new webrev:</p><p class=""><a \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/" moz-do-not-send="true" \
class="">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/</a></p><p class="">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.</p><p class="">Leonid<br class="">
</p>
<div class="moz-cite-prefix">On 6/9/20 6:59 PM, <a \
class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" \
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br class=""> </div>
<blockquote type="cite" \
cite="mid:e6e11733-cdee-b718-7734-ced6aa3ca28c@oracle.com" class=""> <div \
class="moz-cite-prefix">On 6/9/20 12:58, Leonid Mesnik wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:c6bde416-5f50-ba7d-1df5-390dd62118fb@oracle.com" class=""><p \
class="">Hi</p> <br class="">
<div class="moz-cite-prefix">On 6/9/20 12:34 PM, <a \
class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" \
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:11314027-4965-b38b-6bc7-5011515b94ab@oracle.com" class=""> <div \
class="moz-cite-prefix">Hi Leonid,<br class=""> <br class="">
Thank you for taking care about this!<br class="">
It looks good in general.<br class="">
However, I think, a similar return is needed in more
cases.<br class="">
<br class="">
One example:<br class="">
<br class="">
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hots \
pot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html</a><br \
class=""> <br class="">
<pre class=""> 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;
<a name="1" id="anc1" moz-do-not-send="true" class=""></a><span class="new"> 104 \
return;</span> 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;
<a name="2" id="anc2" moz-do-not-send="true" class=""></a><span class="new"> 125 \
return;</span> 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 }</pre>
<br class="">
In the fragment above you added return for JVMTI
GetMethodDeclaringClass error.<br class="">
But GetMethodName and GetClassSignature can be also
problematic as the returned names are printed below.<br class="">
It seems to be more safe and even simpler to add returns
for such cases as well.<br class="">
Otherwise, the code reader is puzzled why there is a
return in one failure case and there is no such return
in another.<br class="">
</div>
</blockquote><p class="">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).<br class="">
</p>
</blockquote>
<br class="">
I do not suggest to fix all the tests but those which you are
already fixing.<br class="">
<br class="">
<br class="">
<blockquote type="cite" \
cite="mid:c6bde416-5f50-ba7d-1df5-390dd62118fb@oracle.com" class=""><div class=""> \
<br class="webkit-block-placeholder"></div><p class="">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.</p>
</blockquote>
<br class="">
If JVMTI functions fail with an error code the results with
symbolic strings must be considered invalid.<br class="">
However, they are used later (the values are printed).<br class="">
It is better to bail out in such cases.<br class="">
It should not be a problem to add similar returns in such
cases.<br class="">
Or do you think it is important to continue execution for some
reason?<br class="">
<br class="">
<blockquote type="cite" \
cite="mid:c6bde416-5f50-ba7d-1df5-390dd62118fb@oracle.com" class=""><p \
class="">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.<br class="">
</p>
</blockquote>
<br class="">
Do you suggest it for the updated tests only or for all the
tests with such problems?<br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<blockquote type="cite" \
cite="mid:c6bde416-5f50-ba7d-1df5-390dd62118fb@oracle.com" class=""><div class=""> \
<br class="webkit-block-placeholder"></div><p class="">Leonid<br class=""> </p>
<blockquote type="cite" \
cite="mid:11314027-4965-b38b-6bc7-5011515b94ab@oracle.com" class=""> <div \
class="moz-cite-prefix"> <br class=""> Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 6/1/20 21:33, Leonid Mesnik wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:bb8a053a-5ed4-1c53-f34d-f81c9d43141e@oracle.com" class="">Hi <br class="">
<br class="">
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. <br class="">
<br class="">
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. <br class="">
<br class="">
webrev: <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/</a> \
<br class=""> <br class="">
bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8242891" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8242891</a> <br \
class=""> <br class="">
Leonid <br class="">
<br class="">
</blockquote>
<br class="">
</blockquote>
</blockquote>
<br class="">
</blockquote>
</blockquote>
<br class="">
</blockquote>
</div>
</div></blockquote></div><br class=""></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic