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/>
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 <[email protected]
<mailto:[email protected]>> 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/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210700
>
> Thanks again for the reviews,
> Jc
--
Thanks,
Jc
