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