RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
JC Beyler
jcbeyler at google.com
Fri Sep 14 20:50:16 UTC 2018
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/
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>
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/>
> > 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>> 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/>
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
> > >
> > > Thanks again for the reviews,
> > > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180914/15220035/attachment-0001.html>
More information about the serviceability-dev
mailing list