RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]
David Holmes
david.holmes at oracle.com
Thu Oct 18 02:26:20 UTC 2018
On 18/10/2018 11:57 AM, JC Beyler wrote:
> Hi all,
>
> @Serguei: no worries for the time taken, I am just two webrevs away from
> conquering the NSK_CPP_STUBs, I might have been a bit pushy for a review
> :-) I apologize!
>
> @David: It's not hotspot style but a lot of those files had that style.
> My CL actually makes it non-conforming all around:
>
> if ( before_we_had_this ) // space after ( + space before )
>
> if (now_we_have_this ) // only space before )
>
> which is kind of weird and Paul's comment was to at least try to not
> make it inconsistent to that point.
>
> I have one webrev to go that will remove NSK_CPP_STUBs and then I'm
> going to fix all the spaces (via
> https://bugs.openjdk.java.net/browse/JDK-8211335) for these .cpp files
> so we won't have the issue anymore.
>
> Does that make sense?
Yes thanks for clarifying.
David
> Thanks!
> Jc
>
>
> On Wed, Oct 17, 2018 at 6:34 PM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> On 18/10/2018 6:01 AM, Hohensee, Paul wrote:
> > Re spaces :). There’s a bunch of places where “(jvmti_env”
> should be
> > “( jvmti_env”, but of course not all of them. I found a bunch.
>
> Why should there be a space after the '(' ? That's not hotspot-style.
> Neither is a space before ')'.
>
> David
>
> > Otherwise, lgtm., no need for another webrev.
> >
> > Paul
> >
> > In hs104t002.cpp
> >
> > - if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> >
> > - &caps) )) {
> >
> > + if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
> >
> > In hs203t003.cpp
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
> >
> > - jvmti_env, klass, &className, &generic) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
> > &className, &generic) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
> > &className, &generic) ) ) {
> >
> > and
> >
> > - if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> > &caps) )) {
> >
> > + if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
> >
> > and
> >
> > - if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
> > thread, &state) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > and
> >
> > - if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
> > thread, &state) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > nsk_printf(" Agent :: Error while getting thread
> state.\n");
> >
> > nsk_jvmti_agentFailed();
> >
> > } else {
> >
> > if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame,
> jvmti,
> > thread) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > …
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
> >
> > and
> >
> > - if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti,
> > thread)) ) {
> >
> > + if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {
> >
> > =>
> >
> > + if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {
> >
> > In hs203t004.cpp
> >
> > - if ( ! NSK_JVMTI_VERIFY (
> NSK_CPP_STUB2(GenerateEvents,
> > jvmti_env,
> >
> > - JVMTI_EVENT_COMPILED_METHOD_LOAD
> ) )) {
> >
> > + if ( ! NSK_JVMTI_VERIFY
> > (jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY (
> > jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
> >
> > and
> >
> > - if ( ! NSK_JVMTI_VERIFY (
> NSK_CPP_STUB3(GetMethodDeclaringClass,
> >
> > - jvmti_env, method, &threadClass) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY
> > (jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY (
> > jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
> >
> > and
> >
> > - if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> > &caps) )) {
> >
> > + if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
> >
> > and
> >
> > - if ( NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti,
> thread)
> > ) ) {
> >
> > + if ( NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {
> >
> > =>
> >
> > + if ( NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {
> >
> > and
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,
> >
> > - thread, &state) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > NSK_COMPLAIN0("#error Agent :: while getting thread's
> state.\n");
> >
> > nsk_jvmti_agentFailed();
> >
> > } else {
> >
> > if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
> >
> > - if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti,
> > thread) ) ){
> >
> > + if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread,
> &state) ) ) {
> >
> > …
> >
> > + if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){
> >
> > In hs204t001.cpp
> >
> > - /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env,
> > klass)) == NULL) {
> >
> > + /* if( (myTestClass =jni_env->NewGlobalRef(klass)) ==
> NULL) {
> >
> > =>
> >
> > + /* if( (myTestClass = jni_env->NewGlobalRef(klass)) ==
> NULL) {
> >
> > and
> >
> > - if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
> > NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))
> >
> > + if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
> > env->NewGlobalRef(klass)) != NULL))
> >
> > nsk_jvmti_setFailStatus();
> >
> > - if (!NSK_JNI_VERIFY(env, (testedThread
> =NSK_CPP_STUB2(NewGlobalRef,
> > env, thread)) != NULL))
> >
> > + if (!NSK_JNI_VERIFY(env, (testedThread
> =env->NewGlobalRef(thread))
> > != NULL))
> >
> > =>
> >
> > + if (!NSK_JNI_VERIFY(env, (testClass = (jclass)
> > env->NewGlobalRef(klass)) != NULL))
> >
> > nsk_jvmti_setFailStatus();
> >
> > …
> >
> > + if (!NSK_JNI_VERIFY(env, (testedThread =
> env->NewGlobalRef(thread))
> > != NULL))
> >
> > and
> >
> > - if (!NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread,
> jvmti,
> > thread))) {
> >
> > + if (!NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread))) {
> >
> > =>
> >
> > + if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
> >
> > In hs204t003.cpp
> >
> > - if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
> > thread, &state)) ){
> >
> > + if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){
> >
> > NSK_DISPLAY0(" Agent :: Error getting thread state.\n");
> >
> > nsk_jvmti_agentFailed();
> >
> > } else {
> >
> > if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
> >
> > NSK_DISPLAY0(" Agent :: Thread state =
> JVMTI_THREAD_STATE_SUSPENDED.\n");
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame,
> jvmti,
> > thread) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
> >
> > NSK_DISPLAY0("#error Agent :: Jvmti failed to
> do popFrame.\n");
> >
> > nsk_jvmti_agentFailed();
> >
> > } else {
> >
> > - if ( ! NSK_JVMTI_VERIFY (
> NSK_CPP_STUB2(ResumeThread,
> > jvmti, thread)) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY
> (jvmti->ResumeThread(thread)) ) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
> &state)) ){
> >
> > …
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
> >
> > …
> >
> > + if ( ! NSK_JVMTI_VERIFY (
> jvmti->ResumeThread(thread)) ) {
> >
> > In hs301t001.cpp
> >
> > - if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> > &caps) )) {
> >
> > + if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
> >
> > In hs301t002.cpp
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> > &caps) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps)
> ) ) {
> >
> > In hs301t003.cpp
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
> >
> > - jvmti_env, klass, &className, &generic) ) ) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
> > &className, &generic) ) ) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
> > &className, &generic) ) ) {
> >
> > and
> >
> > - if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> jvmti,
> > &caps) )) {
> >
> > + if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
> >
> > and
> >
> > - if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
> >
> > - jvmti, &caps) )) {
> >
> > + if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
> >
> > =>
> >
> > + if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps)
> )) {
> >
> > *From: *serviceability-dev
> <serviceability-dev-bounces at openjdk.java.net
> <mailto:serviceability-dev-bounces at openjdk.java.net>>
> > on behalf of JC Beyler <jcbeyler at google.com
> <mailto:jcbeyler at google.com>>
> > *Date: *Tuesday, October 16, 2018 at 4:24 PM
> > *To: *"alexey.menkov at oracle.com
> <mailto:alexey.menkov at oracle.com>" <alexey.menkov at oracle.com
> <mailto:alexey.menkov at oracle.com>>
> > *Cc: *"serviceability-dev at openjdk.java.net
> <mailto:serviceability-dev at openjdk.java.net>"
> > <serviceability-dev at openjdk.java.net
> <mailto:serviceability-dev at openjdk.java.net>>
> > *Subject: *Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from
> > vmTestbase for jvmti/scenarios/[E-M]
> >
> > Hi all,
> >
> > How about on a Tuesday? :)
> >
> > Sneak peak and motivational sentence: after this goes in, we have
> this
> > one <http://cr.openjdk.java.net/%7Ejcbeyler/8212148/webrev.00/>
> which
> > removes the NSK_CPP_STUBs from the tests entirely! :)
> >
> > Jc
> >
> > On Fri, Oct 12, 2018 at 5:37 PM JC Beyler <jcbeyler at google.com
> <mailto:jcbeyler at google.com>
> > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>> wrote:
> >
> > Hi all,
> >
> > Any chance for a second reviewer on a Friday? :)
> >
> > Jc
> >
> > On Thu, Oct 11, 2018 at 11:03 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:
> >
> > got it.
> >
> > LGTM.
> >
> > --alex
> >
> > On 10/10/2018 19:03, JC Beyler wrote:
> > > Hi Alex,
> > >
> > > Thanks for the review! Yes I had seen that too before
> sending
> > this
> > > review out and forked that fix into this:
> > > https://bugs.openjdk.java.net/browse/JDK-8211905
> > >
> > > Which now is merged and I've updated my webrev to reflect
> > this of course.
> > >
> > > My rationale for not doing it here directly is always the
> > same: keeping
> > > the changes to the "spirit" of what the change is
> trying to
> > do. Those
> > > extra casts were a bit out-of-scope and so I just
> forked the
> > fix in 8211905.
> > >
> > > Thanks!
> > > Jc
> > >
> > > On Wed, Oct 10, 2018 at 5:40 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,
> > >
> > > Overall looks good.
> > >
> > > one minor note:
> > >
> > >
> >
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/events/EM06/em06t001/em06t001.cpp:
> > > - jclassName = (jstring) (jstring) (jstring)
> (jstring)
> > (jstring)
> > > (jstring) (jstring) (jstring) (jstring)
> > NSK_CPP_STUB3(CallObjectMethod,
> > > jni_env, klass,
> > > - methodID);
> > > + jclassName = (jstring) (jstring) (jstring)
> (jstring)
> > (jstring)
> > > (jstring) (jstring) (jstring) (jstring)
> > > jni_env->CallObjectMethod(klass,
> > > methodID);
> > >
> > > Please drop multi "(jstring)"
> > >
> > > --alex
> > >
> > > On 10/08/2018 21:21, JC Beyler wrote:
> > > > Hi all,
> > > >
> > > > I am continuing the NSK_CPP_STUB removal with this
> > next webrev.
> > > > Webrev:
> > http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > >
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > > >
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > > > Bug:
> https://bugs.openjdk.java.net/browse/JDK-8211899
> > > >
> > > > The change is still straight-forward though,
> since it
> > is just
> > > doing the
> > > > same NSK_CPP_STUB removal. However when I was
> looking
> > at the
> > > changes, a
> > > > lot of these changes are touching lines with spaces
> > before/after
> > > > parenthesis. I've almost never touched the spaces
> > except if I was
> > > > refactoring by hand the line at the same time. The
> > rationale
> > > being that
> > > > the lines will get fixed a few more times and
> are, at
> > worse,
> > > covered by
> > > > the bug:
> > https://bugs.openjdk.java.net/browse/JDK-8211335, which
> > > I've
> > > > commited to doing.
> > > >
> > > > Two exceptions are here where I pushed out the
> code into
> > > assignments due
> > > > to really long lines and complex if structures:
> > > > -
> jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp
> > > >
> > >
> >
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp.udiff.html>
> > > > -
> > jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp
> > > >
> > >
> >
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp.udiff.html>
> > > >
> > > > And one exception here where a commented line was
> > doing the
> > > out-of-if
> > > > assignment so I just uncommented it and used
> the variable:
> > > > -
> jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp
> > > >
> > >
> >
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp.udiff.html>
> > > >
> > > > I've tested the modified changes on my machine, all
> > modified
> > > tests pass.
> > > >
> > > > Let me know what you think,
> > > > Jc
> > > >
> > > > Ps: 2 more of these and we can say good bye to
> > NSK_CPP_STUB*
> > > >
> > >
> > >
> > >
> > > --
> > >
> > > Thanks,
> > > Jc
> >
> >
> > --
> >
> > Thanks,
> >
> > Jc
> >
> >
> > --
> >
> > Thanks,
> >
> > Jc
> >
>
>
>
> --
>
> Thanks,
> Jc
More information about the serviceability-dev
mailing list