[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-09-18 2:51:35
Message-ID: CAF9BGByG7LE47EDKZ45R5zmRb-RKeeDqFwzMJOLwnRMhmAFaUw () mail ! gmail ! com
[Download RAW message or body]
Hi all,
Thanks David, I pushed the webrev after re-testing the unit subtests.
Have all a great evening,
Jc
On Mon, Sep 17, 2018 at 3:47 PM David Holmes <david.holmes@oracle.com>
wrote:
> I'm fine with the code the way it is.
>
> Thanks,
> David
>
> On 18/09/2018 4:08 AM, Alex Menkov wrote:
> > I raised the point because I remember I saw similar issue.
> > Finally I found the issue it and it was about JNIEnv.
> > So there is no problem here (as tests creates only a single jvmtiEnv).
> > Anyway I think it would be better to use jvmtiEnv passed to callbacks
> > (then it remains correct even is other jvmtiEnv is created).
> >
> > --alex
> >
> > On 09/17/2018 09:14, JC Beyler wrote:
> >> Hi David,
> >>
> >> I think it is fine to leave the caching in the most tests I looked
> >> because they want to do JVMTI calls where there is jvmtiEnv* passed
> >> in. Would you rather I revert the rawmonitor changes to where it is
> >> still using the cached one instead of the one passed in by the call?
> >>
> >> Let me know,
> >> Jc
> >>
> >> On Sun, Sep 16, 2018 at 9:15 PM David Holmes <david.holmes@oracle.com
> >> <mailto:david.holmes@oracle.com>> wrote:
> >>
> >> I took a look at it all and it seems okay, though the use of the
> >> cached
> >> jvmtiEnv pointer did not really need to be changed. As per the spec:
> >>
> >> "JVM TI environments work across threads"
> >>
> >> The caching and its use is somewhat hard to understand without
> seeing
> >> where all the call paths are for the functions that still used the
> >> cached version.
> >>
> >> It would have been simpler to address the caching issue (if it
> >> needs to
> >> be addressed) separately.
> >>
> >> Cheers,
> >> David
> >>
> >> On 15/09/2018 7:30 AM, Alex Menkov wrote:
> >> > Hi Jc,
> >> >
> >> > I looked only at rawmonitor.cpp (I suppose nothing other has been
> >> changed).
> >> > Looks good.
> >> >
> >> > --alex
> >> >
> >> > On 09/14/2018 13:50, JC Beyler wrote:
> >> >> Hi Alex,
> >> >>
> >> >> Ok I understand now what you mean. I just did a double check on
> >> files
> >> >> that had global definitions of jvmtiEnv across the tests (not
> >> complete
> >> >> but I looked at any file that has a grep for "^jvmtiEnv") and
> >> those
> >> >> were the only case where this happens.
> >> >>
> >> >> Here is a new version:
> >> >> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.02/
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/>
> >> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/>
> >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
> >> >>
> >> >> Let me know what you think and sorry I misunderstood what you
> >> meant,
> >> >> Jc
> >> >>
> >> >> On Fri, Sep 14, 2018 at 10:52 AM Alex Menkov
> >> <alexey.menkov@oracle.com <mailto:alexey.menkov@oracle.com>
> >> >> <mailto:alexey.menkov@oracle.com
> >> <mailto:alexey.menkov@oracle.com>>> wrote:
> >> >>
> >> >> Hi Jc,
> >> >>
> >> >> On 09/13/2018 20:05, JC Beyler wrote:
> >> >> > Thanks Alexey for the review, I fixed all the " ," issues
> >> that
> >> >> the patch
> >> >> > changed but there are still at least 29 files that seem
> >> to have
> >> >> that
> >> >> > issue in the vmTestbase that were not touched by this
> >> webrev. I
> >> >> imagine
> >> >> > we can do a refactoring in another webrev (want me to
> >> file it?)
> >> >> or we
> >> >> > can try to handle them when we refactor the tests to move
> >> them
> >> >> out of
> >> >> > vmTestbase.
> >> >>
> >> >> I don't think we need to fix this minor style issues - I
> >> asked to fix
> >> >> them just because your fix touches the lines.
> >> >>
> >> >> Regarding jvmti/jvmti_env mix:
> >> >> Looks like you are right about
> >> <...>/timers/JvmtiTest/JvmtiTest.cpp
> >> >> (actually if JNI_ENV_ARG didn't drop the 1st arg, the code
> >> would just
> >> >> fail to compile as jvmti_env is undefined in some cases).
> >> >>
> >> >> But the same issues in
> >> <...>/functions/rawmonitor/rawmonitor.cpp
> >> >> needs
> >> >> to be fixed.
> >> >> As I wrote before if jvmtiEnv is used in JVMTI callback, it
> >> should
> >> >> use
> >> >> jvmtiEnv passed to the callback (callback may be called on a
> >> >> different
> >> >> thread and in the case jvmti if different from jvmti_env):
> >> >>
> >> >> void JNICALL vmStart(jvmtiEnv *jvmti_env, JNIEnv *env) {
> >> >> jvmtiError res;
> >> >> - res =
> >> >>
> >> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,
> >> >> &main_thread));
> >> >> + res = jvmti->GetCurrentThread(&main_thread);
> >> >>
> >> >> should be
> >> >> + res = jvmti_env->GetCurrentThread(&main_thread);
> >> >>
> >> >> the same for other callbacks in rawmonitor.cpp
> >> >>
> >> >> --alex
> >> >>
> >> >> >
> >> >> > The new webrev is here:
> >> >> > Webrev:
> >> http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.01/
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/>
> >> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/>
> >> >> >
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/>
> >> >> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
> >> >> >
> >> >> > Good catch on the change here:
> >> >> > - res =
> >> >> >
> >> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,
> >> >> > &main_thread));
> >> >> > + res = jvmti->GetCurrentThread(&main_thread);
> >> >> >
> >> >> > You are right that the change from Igor introduced this
> >> weird
> >> >> part where
> >> >> > jvmti and jvmti_env are seemingly used at the same time.
> >> Turns
> >> >> out that
> >> >> > for C++, JVMTI_ENV_ARG/JVMTI_ENV_PTR is transformed into:
> >> >> >
> >> >> > -#define JNI_ENV_PTR(x) x
> >> >> > -#define JNI_ENV_ARG(x, y) y
> >> >> >
> >> >> > ..
> >> >> >
> >> >> > -#define JVMTI_ENV_PTR JNI_ENV_PTR
> >> >> > -#define JVMTI_ENV_ARG JNI_ENV_ARG
> >> >> >
> >> >> > So you are right that actually it is weird but it all
> >> works out:
> >> >> >
> >> >> > - res =
> >> >> >
> >> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,
> >> >> > &main_thread));
> >> >> >
> >> >> > -> The JVMTI_ENV_PTR is JNI_ENV_PTR which is identity so
> >> >> JVMTI_ENV_PTR(jvmti) -> jvmti
> >> >> >
> >> >> > -> The JVMT_ENV_ARG ignores the first argument so
> >> >> JVMTI_ENV_ARG(jvmti_env, &main_thread) -> &main_thread
> >> >> >
> >> >> > So my transformation is correct; turns out that Igor's
> >> >> transformation was wrong but because things were in C++,
> >> >> >
> >> >> > the undeclared jvmti_env was just ignored.
> >> >> >
> >> >> >
> >> >> > So apart from the case where I missed something I think
> >> we are
> >> >> good. Let me know what you think,
> >> >> >
> >> >> > Jc
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Thu, Sep 13, 2018 at 5:32 PM Alex Menkov
> >> >> <alexey.menkov@oracle.com <mailto:alexey.menkov@oracle.com>
> >> <mailto:alexey.menkov@oracle.com <mailto:alexey.menkov@oracle.com>>
> >> >> > <mailto:alexey.menkov@oracle.com
> >> <mailto:alexey.menkov@oracle.com>
> >> >> <mailto:alexey.menkov@oracle.com
> >> <mailto:alexey.menkov@oracle.com>>>> wrote:
> >> >> >
> >> >> > Hi Jc,
> >> >> >
> >> >> > Some notes:
> >> >> > <...>/MethodBind/JvmtiTest/JvmtiTest.cpp
> >> >> > and
> >> >> > <...>/StackTrace/JvmtiTest/JvmtiTest.cpp
> >> >> > have several places with extra space before comma
> >> like:
> >> >> > - ret =
> >> >> JVMTI_ENV_PTR(jvmti)->GetStackTrace(JVMTI_ENV_ARG(jvmti,
> >> >> > thr), 0, max_count , stack_buffer, &count);
> >> >> > + ret = jvmti->GetStackTrace(thr, 0, max_count ,
> >> >> stack_buffer,
> >> >> > &count);
> >> >> >
> >> >> > <...>/functions/rawmonitor/rawmonitor.cpp
> >> >> > and
> >> >> > <...>/timers/JvmtiTest/JvmtiTest.cpp
> >> >> > have several suspicious changes when JVMTI_ENV_PTR
> and
> >> >> JVMTI_ENV_ARG
> >> >> > have different arguments (that's certainly wrong, but
> >> needs
> >> >> to re
> >> >> > resolved correctly):
> >> >> > - res =
> >> >> >
> >> >> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,
> >> >> > &main_thread));
> >> >> > + res = jvmti->GetCurrentThread(&main_thread);
> >> >> >
> >> >> > JVMTI_ENV_PTR(jvmti) is an address of the function
> >> in the
> >> >> vtable, and
> >> >> > JVMTI_ENV_ARG(jvmti_env, ...) is a C++ "this"
> pointer.
> >> >> > So I'd expect that this should be
> >> >> > + res = +
> >> jvmti_env->GetCurrentThread(&main_thread);
> >> >> >
> >> >> > Looking at timers/JvmtiTest/JvmtiTest.cpp history
> >> looks like
> >> >> >
> >> JVMTI_ENV_PTR(jvmti)-><func>(JVMTI_ENV_ARG(jvmti_env, ...
> >> >> changes were
> >> >> > introduced recently by the fix for "8209611: use C++
> >> >> compiler for
> >> >> > hotspot tests".
> >> >> >
> >> >> > /functions/rawmonitor/rawmonitor.cpp had such wrong
> >> >> statements before,
> >> >> > so they should be revised carefully.
> >> >> > AFAIU if JVMTI dunction is called from some callback
> >> where
> >> >> jvmtiEnv is
> >> >> > passed, the passed value should be used.
> >> >> >
> >> >> > --alex
> >> >> >
> >> >> > On 09/13/2018 13:26, JC Beyler wrote:
> >> >> > > Hi all,
> >> >> > >
> >> >> > > We have arrived to the last webrev for removing
> >> the JNI_ENV
> >> >> > macros from
> >> >> > > the vmTestbase:
> >> >> > >
> >> >> > > Webrev:
> >> >> http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.00/
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/>
> >> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/>
> >> >> >
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/>
> >> >> > >
> >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/>
> >> >> > > Bug:
> >> https://bugs.openjdk.java.net/browse/JDK-8210700
> >> >> > >
> >> >> > > Thanks again for the reviews,
> >> >> > > Jc
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> >
> >> >> > Thanks,
> >> >> > Jc
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >>
> >> >> Thanks,
> >> >> Jc
> >>
> >>
> >>
> >> --
> >>
> >> Thanks,
> >> Jc
>
--
Thanks,
Jc
[Attachment #3 (text/html)]
<div dir="ltr"><div dir="ltr">Hi all,<div><br></div><div>Thanks David, I pushed the \
webrev after re-testing the unit subtests.</div><div><br></div><div>Have all a great \
evening,</div><div>Jc</div><div></div></div></div><br><div class="gmail_quote"><div \
dir="ltr">On Mon, Sep 17, 2018 at 3:47 PM David Holmes <<a \
href="mailto:david.holmes@oracle.com">david.holmes@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">I'm fine with the code the way \
it is.<br> <br>
Thanks,<br>
David<br>
<br>
On 18/09/2018 4:08 AM, Alex Menkov wrote:<br>
> I raised the point because I remember I saw similar issue.<br>
> Finally I found the issue it and it was about JNIEnv.<br>
> So there is no problem here (as tests creates only a single jvmtiEnv).<br>
> Anyway I think it would be better to use jvmtiEnv passed to callbacks <br>
> (then it remains correct even is other jvmtiEnv is created).<br>
> <br>
> --alex<br>
> <br>
> On 09/17/2018 09:14, JC Beyler wrote:<br>
>> Hi David,<br>
>><br>
>> I think it is fine to leave the caching in the most tests I looked <br>
>> because they want to do JVMTI calls where there is jvmtiEnv* passed <br>
>> in. Would you rather I revert the rawmonitor changes to where it is <br>
>> still using the cached one instead of the one passed in by the call?<br>
>><br>
>> Let me know,<br>
>> Jc<br>
>><br>
>> On Sun, Sep 16, 2018 at 9:15 PM David Holmes <<a \
href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a> \
<br> >> <mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>>> wrote:<br> >><br>
>> I took a look at it all and it seems okay, though the use of the <br>
>> cached<br>
>> jvmtiEnv pointer did not really need to be changed. As per the \
spec:<br> >><br>
>> "JVM TI environments work across threads"<br>
>><br>
>> The caching and its use is somewhat hard to understand without \
seeing<br> >> where all the call paths are for the functions that still \
used the<br> >> cached version.<br>
>><br>
>> It would have been simpler to address the caching issue (if it <br>
>> needs to<br>
>> be addressed) separately.<br>
>><br>
>> Cheers,<br>
>> David<br>
>><br>
>> On 15/09/2018 7:30 AM, Alex Menkov wrote:<br>
>> > Hi Jc,<br>
>> ><br>
>> > I looked only at rawmonitor.cpp (I suppose nothing other has \
been<br> >> changed).<br>
>> > Looks good.<br>
>> ><br>
>> > --alex<br>
>> ><br>
>> > On 09/14/2018 13:50, JC Beyler wrote:<br>
>> >> Hi Alex,<br>
>> >><br>
>> >> Ok I understand now what you mean. I just did a double \
check on<br> >> files<br>
>> >> that had global definitions of jvmtiEnv across the tests \
(not<br> >> complete<br>
>> >> but I looked at any file that has a grep for \
"^jvmtiEnv") and <br> >> those<br>
>> >> were the only case where this happens.<br>
>> >><br>
>> >> Here is a new version:<br>
>> >> Webrev: <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.02/</a><br> \
>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/</a>><br> \
>> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.02/</a>><br> \
>> >> Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8210700" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210700</a><br> >> \
>><br> >> >> Let me know what you think and sorry I \
misunderstood what you <br> >> meant,<br>
>> >> Jc<br>
>> >><br>
>> >> On Fri, Sep 14, 2018 at 10:52 AM Alex Menkov<br>
>> <<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >> >> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>> wrote:<br> >> \
>><br> >> >> Hi Jc,<br>
>> >><br>
>> >> On 09/13/2018 20:05, JC Beyler wrote:<br>
>> >> > Thanks Alexey for the review, I fixed all \
the " ," issues<br> >> that<br>
>> >> the patch<br>
>> >> > changed but there are still at least 29 \
files that seem<br> >> to have<br>
>> >> that<br>
>> >> > issue in the vmTestbase that were not \
touched by this<br> >> webrev. I<br>
>> >> imagine<br>
>> >> > we can do a refactoring in another webrev \
(want me to<br> >> file it?)<br>
>> >> or we<br>
>> >> > can try to handle them when we refactor the \
tests to move<br> >> them<br>
>> >> out of<br>
>> >> > vmTestbase.<br>
>> >><br>
>> >> I don't think we need to fix this minor style \
issues - I<br> >> asked to fix<br>
>> >> them just because your fix touches the lines.<br>
>> >><br>
>> >> Regarding jvmti/jvmti_env mix:<br>
>> >> Looks like you are right about<br>
>> <...>/timers/JvmtiTest/JvmtiTest.cpp<br>
>> >> (actually if JNI_ENV_ARG didn't drop the 1st \
arg, the code<br> >> would just<br>
>> >> fail to compile as jvmti_env is undefined in some \
cases).<br> >> >><br>
>> >> But the same issues in<br>
>> <...>/functions/rawmonitor/rawmonitor.cpp<br>
>> >> needs<br>
>> >> to be fixed.<br>
>> >> As I wrote before if jvmtiEnv is used in JVMTI \
callback, it<br> >> should<br>
>> >> use<br>
>> >> jvmtiEnv passed to the callback (callback may be \
called on a<br> >> >> different<br>
>> >> thread and in the case jvmti if different from \
jvmti_env):<br> >> >><br>
>> >> void JNICALL vmStart(jvmtiEnv *jvmti_env, JNIEnv \
*env) {<br> >> >> jvmtiError res;<br>
>> >> - res =<br>
>> >> <br>
>> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,<br>
>> >> &main_thread));<br>
>> >> + res = \
jvmti->GetCurrentThread(&main_thread);<br> >> >><br>
>> >> should be<br>
>> >> + res = \
jvmti_env->GetCurrentThread(&main_thread);<br> >> >><br>
>> >> the same for other callbacks in rawmonitor.cpp<br>
>> >><br>
>> >> --alex<br>
>> >><br>
>> >> ><br>
>> >> > The new webrev is here:<br>
>> >> > Webrev:<br>
>> <a href="http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.01/</a><br>
>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/</a>><br> \
>> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/</a>><br> \
>> >> > <br> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.01/</a>><br> \
>> >> > Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8210700" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210700</a><br> >> \
>> ><br> >> >> > Good catch on the \
change here:<br> >> >> > - res =<br>
>> >> ><br>
>> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,<br>
>> >> > &main_thread));<br>
>> >> > + res = \
jvmti->GetCurrentThread(&main_thread);<br> >> >> \
><br> >> >> > You are right that the change from \
Igor introduced this <br> >> weird<br>
>> >> part where<br>
>> >> > jvmti and jvmti_env are seemingly used at \
the same time.<br> >> Turns<br>
>> >> out that<br>
>> >> > for C++, JVMTI_ENV_ARG/JVMTI_ENV_PTR is \
transformed into:<br> >> >> ><br>
>> >> > -#define JNI_ENV_PTR(x) x<br>
>> >> > -#define JNI_ENV_ARG(x, y) y<br>
>> >> ><br>
>> >> > ..<br>
>> >> ><br>
>> >> > -#define JVMTI_ENV_PTR JNI_ENV_PTR<br>
>> >> > -#define JVMTI_ENV_ARG JNI_ENV_ARG<br>
>> >> ><br>
>> >> > So you are right that actually it is weird \
but it all<br> >> works out:<br>
>> >> ><br>
>> >> > - res =<br>
>> >> ><br>
>> JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,<br>
>> >> > &main_thread));<br>
>> >> ><br>
>> >> > -> The JVMTI_ENV_PTR is JNI_ENV_PTR which \
is identity so<br> >> >> JVMTI_ENV_PTR(jvmti) -> \
jvmti<br> >> >> ><br>
>> >> > -> The JVMT_ENV_ARG ignores the first \
argument so<br> >> >> JVMTI_ENV_ARG(jvmti_env, \
&main_thread) -> &main_thread<br> >> >> \
><br> >> >> > So my transformation is correct; \
turns out that Igor's<br> >> >> transformation was \
wrong but because things were in C++,<br> >> >> \
><br> >> >> > the undeclared jvmti_env was just \
ignored.<br> >> >> ><br>
>> >> ><br>
>> >> > So apart from the case where I missed \
something I think<br> >> we are<br>
>> >> good. Let me know what you think,<br>
>> >> ><br>
>> >> > Jc<br>
>> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> > On Thu, Sep 13, 2018 at 5:32 PM Alex \
Menkov<br> >> >> <<a \
href="mailto:alexey.menkov@oracle.com" target="_blank">alexey.menkov@oracle.com</a> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" target="_blank">alexey.menkov@oracle.com</a> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >> >> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >> >> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>>> wrote:<br> >> \
>> ><br> >> >> > Hi Jc,<br>
>> >> ><br>
>> >> > Some notes:<br>
>> >> > \
<...>/MethodBind/JvmtiTest/JvmtiTest.cpp<br> >> >> \
> and<br> >> >> > \
<...>/StackTrace/JvmtiTest/JvmtiTest.cpp<br> >> >> \
> have several places with extra space before comma <br> >> like:<br>
>> >> > - ret =<br>
>> >> \
JVMTI_ENV_PTR(jvmti)->GetStackTrace(JVMTI_ENV_ARG(jvmti,<br> >> \
>> > thr), 0, max_count , stack_buffer, &count);<br> \
>> >> > + ret = \
jvmti->GetStackTrace(thr, 0, max_count ,<br> >> >> \
stack_buffer,<br> >> >> > &count);<br>
>> >> ><br>
>> >> > \
<...>/functions/rawmonitor/rawmonitor.cpp<br> >> >> \
> and<br> >> >> > \
<...>/timers/JvmtiTest/JvmtiTest.cpp<br> >> >> \
> have several suspicious changes when JVMTI_ENV_PTR and<br> >> \
>> JVMTI_ENV_ARG<br> >> >> > \
have different arguments (that's certainly wrong, but<br> >> \
needs<br> >> >> to re<br>
>> >> > resolved correctly):<br>
>> >> > - res =<br>
>> >> ><br>
>> >> \
JVMTI_ENV_PTR(jvmti)->GetCurrentThread(JVMTI_ENV_ARG(jvmti_env,<br> >> \
>> > &main_thread));<br> >> >> \
> + res = jvmti->GetCurrentThread(&main_thread);<br> >> \
>> ><br> >> >> > \
JVMTI_ENV_PTR(jvmti) is an address of the function <br> >> in the<br>
>> >> vtable, and<br>
>> >> > JVMTI_ENV_ARG(jvmti_env, ...) is a \
C++ "this" pointer.<br> >> >> > So \
I'd expect that this should be<br> >> >> > \
+ res = + <br> >> jvmti_env->GetCurrentThread(&main_thread);<br>
>> >> ><br>
>> >> > Looking at \
timers/JvmtiTest/JvmtiTest.cpp history<br> >> looks like<br>
>> >> > <br>
>> JVMTI_ENV_PTR(jvmti)-><func>(JVMTI_ENV_ARG(jvmti_env, ...<br>
>> >> changes were<br>
>> >> > introduced recently by the fix for \
"8209611: use C++<br> >> >> compiler for<br>
>> >> > hotspot tests".<br>
>> >> ><br>
>> >> > /functions/rawmonitor/rawmonitor.cpp \
had such wrong<br> >> >> statements before,<br>
>> >> > so they should be revised \
carefully.<br> >> >> > AFAIU if JVMTI \
dunction is called from some callback<br> >> where<br>
>> >> jvmtiEnv is<br>
>> >> > passed, the passed value should be \
used.<br> >> >> ><br>
>> >> > --alex<br>
>> >> ><br>
>> >> > On 09/13/2018 13:26, JC Beyler \
wrote:<br> >> >> > > Hi all,<br>
>> >> > ><br>
>> >> > > We have arrived to the last \
webrev for removing<br> >> the JNI_ENV<br>
>> >> > macros from<br>
>> >> > > the vmTestbase:<br>
>> >> > ><br>
>> >> > > Webrev:<br>
>> >> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210700/webrev.00/</a><br> \
>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/</a>><br> \
>> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/</a>><br> \
>> >> > <br> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/</a>><br> \
>> >> > ><br> >> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210700/webrev.00/</a>><br> \
>> >> > > Bug: <br> >> <a \
href="https://bugs.openjdk.java.net/browse/JDK-8210700" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210700</a><br> >> \
>> > ><br> >> >> > \
> Thanks again for the reviews,<br> >> >> > \
> Jc<br> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> > --<br>
>> >> ><br>
>> >> > Thanks,<br>
>> >> > Jc<br>
>> >><br>
>> >><br>
>> >><br>
>> >> --<br>
>> >><br>
>> >> Thanks,<br>
>> >> Jc<br>
>><br>
>><br>
>><br>
>> -- <br>
>><br>
>> Thanks,<br>
>> Jc<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic