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

JC Beyler jcbeyler at google.com
Tue Sep 18 02:51:35 UTC 2018


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 <david.holmes at oracle.com>
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 <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
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180917/71242d65/attachment-0001.html>


More information about the serviceability-dev mailing list