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

Alex Menkov alexey.menkov at oracle.com
Mon Sep 17 18:08:37 UTC 2018


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 <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> 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
>     <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,
>      >>
>      >>     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
>      >>     <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
>     <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
>      >>      > <mailto: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/>
>      >>      >      >
>     <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


More information about the serviceability-dev mailing list