RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
Alex Menkov
alexey.menkov at oracle.com
Fri Sep 14 21:30:42 UTC 2018
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/>
> 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>> 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/>
> > 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>>> 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/>
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
> > >
> > > Thanks again for the reviews,
> > > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
>
>
> --
>
> Thanks,
> Jc
More information about the serviceability-dev
mailing list