RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
Alex Menkov
alexey.menkov at oracle.com
Mon Sep 17 18:08:37 UTC 2018
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
More information about the serviceability-dev
mailing list