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

[Alex Menkov]

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