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.hol...@oracle.com
<mailto:david.hol...@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.men...@oracle.com <mailto:alexey.men...@oracle.com>
>> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@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.men...@oracle.com <mailto:alexey.men...@oracle.com>
<mailto:alexey.men...@oracle.com <mailto:alexey.men...@oracle.com>>
>> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
>> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@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
