RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

JC Beyler
Hi all,

We have arrived to the last webrev for removing the JNI_ENV macros from the vmTestbase:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210700

Thanks again for the reviews,
Jc
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

Chris Plummer
Hi JC,

Looks good. Just wanted to point out a couple of argument indentation
choices you made that I like:

  280     ret = jvmti->Allocate(sizeof(jvmtiThreadInfo) * threads_count,
  281                           (unsigned char**)&thread_info);

  304     ret = jvmti->GetThreadListStackTraces(
  305         threads_count, thread_list, MAX_FRAMES_CNT, &stack_buf2);

I would have done these the same way, and is what I was trying to get
across as my preferred way of handling multi-line argument listsbased on
what column the first argument ends up in.

thanks,

Chris

On 9/13/18 1:26 PM, 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



Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

Alex Menkov-2
In reply to this post by JC Beyler
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

JC Beyler
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:

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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

Alex Menkov-2
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 <[hidden email]
> <mailto:[hidden email]>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

JC Beyler
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:

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 <[hidden email]> 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 <[hidden email]
> <mailto:[hidden email]>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

Alex Menkov-2
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 <[hidden email]
> <mailto:[hidden email]>> 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
>     <[hidden email] <mailto:[hidden email]>
>      > <mailto:[hidden email]
>     <mailto:[hidden email]>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

serguei.spitsyn@oracle.com
In reply to this post by JC Beyler
Hi Jc,

Looks good to me.

Thanks,
Serguei


On 9/13/18 13:26, JC Beyler wrote:
Hi all,

We have arrived to the last webrev for removing the JNI_ENV macros from the vmTestbase:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210700

Thanks again for the reviews,
Jc

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

David Holmes
In reply to this post by Alex Menkov-2
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/>
>> 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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>
>>      > <mailto:[hidden email]
>>     <mailto:[hidden email]>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

JC Beyler
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 <[hidden email]> 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/>
>> 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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>
>>      > <mailto:[hidden email]
>>     <mailto:[hidden email]>>> 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


--

Thanks,
Jc
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

Alex Menkov-2
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 <[hidden email]
> <mailto:[hidden email]>> 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
>     <[hidden email] <mailto:[hidden email]>
>      >> <mailto:[hidden email]
>     <mailto:[hidden email]>>> 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
>      >>     <[hidden email] <mailto:[hidden email]>
>     <mailto:[hidden email] <mailto:[hidden email]>>
>      >>      > <mailto:[hidden email]
>     <mailto:[hidden email]>
>      >>     <mailto:[hidden email]
>     <mailto:[hidden email]>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

David Holmes
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>
>>      >> <mailto:[hidden email]
>>     <mailto:[hidden email]>>> 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
>>      >>     <[hidden email] <mailto:[hidden email]>
>>     <mailto:[hidden email] <mailto:[hidden email]>>
>>      >>      > <mailto:[hidden email]
>>     <mailto:[hidden email]>
>>      >>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests

JC Beyler
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 <[hidden email]> 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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>
>>      >> <mailto:[hidden email]
>>     <mailto:[hidden email]>>> 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
>>      >>     <[hidden email] <mailto:[hidden email]>
>>     <mailto:[hidden email] <mailto:[hidden email]>>
>>      >>      > <mailto:[hidden email]
>>     <mailto:[hidden email]>
>>      >>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>>> 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