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

[David Holmes]

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 <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