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 03:05:49 UTC 2018
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.
The new webrev is here:
Webrev: http://cr.openjdk.java.net/~jcbeyler/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>
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/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
> >
> > Thanks again for the reviews,
> > Jc
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180913/3d57842b/attachment.html>
More information about the serviceability-dev
mailing list