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 01:34:01 UTC 2018


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> 
> on behalf of JC Beyler <jcbeyler at google.com>
> *Date: *Tuesday, October 16, 2018 at 4:24 PM
> *To: *"alexey.menkov at oracle.com" <alexey.menkov at oracle.com>
> *Cc: *"serviceability-dev at openjdk.java.net" 
> <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>> 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>> 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>>> 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/>
>          >      > 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
> 


More information about the serviceability-dev mailing list