RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]
JC Beyler
jcbeyler at google.com
Thu Oct 18 01:57:38 UTC 2018
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?
Thanks!
Jc
On Wed, Oct 17, 2018 at 6:34 PM David Holmes <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>
>
> > 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
> >
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181017/e67302d2/attachment-0001.html>
More information about the serviceability-dev
mailing list