RFR (M) 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
David Holmes
david.holmes at oracle.com
Mon Sep 17 22:47:22 UTC 2018
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
More information about the serviceability-dev
mailing list