RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
JC Beyler
jcbeyler at google.com
Tue Sep 18 02:51:35 UTC 2018
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 at 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 at oracle.com
> >> <mailto:david.holmes at 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 at oracle.com <mailto:alexey.menkov at oracle.com>
> >> >> <mailto:alexey.menkov at oracle.com
> >> <mailto:alexey.menkov at 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 at oracle.com <mailto:alexey.menkov at oracle.com>
> >> <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
> >> >> > <mailto:alexey.menkov at oracle.com
> >> <mailto:alexey.menkov at oracle.com>
> >> >> <mailto:alexey.menkov at oracle.com
> >> <mailto:alexey.menkov at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180917/71242d65/attachment-0001.html>
More information about the serviceability-dev
mailing list