Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, we have some tests checking the number of jdwp packets and they've detected a regression here: com.jetbrains.jdi.ObjectReferenceImpl#validateAssignment now always requests referenceType (requiring one more jdwp packet if value is not yet cached), previously it happened only for arrays. Simple fix like this solves the issue: === --- src/main/java/com/jetbrains/jdi/ObjectReferenceImpl.java (revision d96cab0ec2eeb791cceb7884341c56496cc026b9) +++ src/main/java/com/jetbrains/jdi/ObjectReferenceImpl.java (revision 10a78a742d17338778e0226990ad0029a71cbf50) @@ -647,11 +647,10 @@ */ JNITypeParser destSig = new JNITypeParser(destination.signature()); - JNITypeParser sourceSig = new JNITypeParser(type().signature()); if (destSig.isPrimitive()) { throw new InvalidTypeException("Can't assign object value to primitive"); } - if (destSig.isArray() && !sourceSig.isArray()) { + if (destSig.isArray() && !new JNITypeParser(type().signature()).isArray()) { throw new InvalidTypeException("Can't assign non-array value to an array"); } if (destSig.isVoid()) { If possible - please push the fix. Thanks, Egor On 19-May-20 19:32, Daniil Titov wrote: Hi Chris and Serguei, Thank you for reviewing this change. Best regards, Daniil On 5/18/20, 12:41 PM, "Chris Plummer" wrote: Looks good. thanks, Chris On 5/14/20 1:33 PM, Daniil Titov wrote: > Hi Serguei and Chris, > > Please review a new version of the change [1] that addresses your comments. > > Testing: Mach5 tier1-tier5 tests successfully passed. > > Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed > we really don't need it, so I withdrew this CR. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > [3] https://bugs.openjdk.java.net/browse/JDK-8245057 > > Thank you, > Daniil > > > From: "serguei.spit...@oracle.com" > Date: Monday, May 11, 2020 at 11:53 AM > To: Daniil Titov , serviceability-dev > Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools > > Hi Daniil, > > It looks pretty good in general. > A couple of nits below. > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html > +void *cursor; > +jbyte argumentTag; > +jint argIndex = 0; > +jvalue *argument = request->arguments;; > . . . > void *cursor; > jint argIndex = 0; > +jbyte argumentTag; > jvalue *argument = request->arguments; > > It is better if the local variables 'cursor' and 'argumentTag' get initialized. > There is double semicolon: "arguments;;" > > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html >43 static inline jbyte basicType(const char *signature) { > > It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. > > It is more safe to run tier5 as well. > > Thanks, > Serguei > > > On 5/9/20 09:29, Daniil Titov wrote: > Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. > > Testing: Mach5 tier1-tier3 tests successfully passed. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > > Thank you, > Daniil > > > > > > -- Egor Ushakov Software Developer JetBrains http://www.jetbrains.com The Drive to Develop
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Chris and Serguei, Thank you for reviewing this change. Best regards, Daniil On 5/18/20, 12:41 PM, "Chris Plummer" wrote: Looks good. thanks, Chris On 5/14/20 1:33 PM, Daniil Titov wrote: > Hi Serguei and Chris, > > Please review a new version of the change [1] that addresses your comments. > > Testing: Mach5 tier1-tier5 tests successfully passed. > > Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed > we really don't need it, so I withdrew this CR. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > [3] https://bugs.openjdk.java.net/browse/JDK-8245057 > > Thank you, > Daniil > > > From: "serguei.spit...@oracle.com" > Date: Monday, May 11, 2020 at 11:53 AM > To: Daniil Titov , serviceability-dev > Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools > > Hi Daniil, > > It looks pretty good in general. > A couple of nits below. > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html > +void *cursor; > +jbyte argumentTag; > +jint argIndex = 0; > +jvalue *argument = request->arguments;; > . . . > void *cursor; > jint argIndex = 0; > +jbyte argumentTag; > jvalue *argument = request->arguments; > > It is better if the local variables 'cursor' and 'argumentTag' get initialized. > There is double semicolon: "arguments;;" > > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html >43 static inline jbyte basicType(const char *signature) { > > It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. > > It is more safe to run tier5 as well. > > Thanks, > Serguei > > > On 5/9/20 09:29, Daniil Titov wrote: > Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. > > Testing: Mach5 tier1-tier3 tests successfully passed. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > > Thank you, > Daniil > > > > > >
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Looks good. thanks, Chris On 5/14/20 1:33 PM, Daniil Titov wrote: Hi Serguei and Chris, Please review a new version of the change [1] that addresses your comments. Testing: Mach5 tier1-tier5 tests successfully passed. Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed we really don't need it, so I withdrew this CR. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 [3] https://bugs.openjdk.java.net/browse/JDK-8245057 Thank you, Daniil From: "serguei.spit...@oracle.com" Date: Monday, May 11, 2020 at 11:53 AM To: Daniil Titov , serviceability-dev Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools Hi Daniil, It looks pretty good in general. A couple of nits below. http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html +void *cursor; +jbyte argumentTag; +jint argIndex = 0; +jvalue *argument = request->arguments;; . . . void *cursor; jint argIndex = 0; +jbyte argumentTag; jvalue *argument = request->arguments; It is better if the local variables 'cursor' and 'argumentTag' get initialized. There is double semicolon: "arguments;;" http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html 43 static inline jbyte basicType(const char *signature) { It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. It is more safe to run tier5 as well. Thanks, Serguei On 5/9/20 09:29, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, Thank you for the update! It looks good to me. > Regarding CR for the JDWP spec issues I guess, you wanted to say "CSR", not CR. :) Thanks, Serguei On 5/14/20 13:33, Daniil Titov wrote: Hi Serguei and Chris, Please review a new version of the change [1] that addresses your comments. Testing: Mach5 tier1-tier5 tests successfully passed. Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed we really don't need it, so I withdrew this CR. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 [3] https://bugs.openjdk.java.net/browse/JDK-8245057 Thank you, Daniil From: "serguei.spit...@oracle.com" Date: Monday, May 11, 2020 at 11:53 AM To: Daniil Titov , serviceability-dev Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools Hi Daniil, It looks pretty good in general. A couple of nits below. http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html +void *cursor; +jbyte argumentTag; +jint argIndex = 0; +jvalue *argument = request->arguments;; . . . void *cursor; jint argIndex = 0; +jbyte argumentTag; jvalue *argument = request->arguments; It is better if the local variables 'cursor' and 'argumentTag' get initialized. There is double semicolon: "arguments;;" http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html 43 static inline jbyte basicType(const char *signature) { It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. It is more safe to run tier5 as well. Thanks, Serguei On 5/9/20 09:29, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Serguei and Chris, Please review a new version of the change [1] that addresses your comments. Testing: Mach5 tier1-tier5 tests successfully passed. Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed we really don't need it, so I withdrew this CR. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 [3] https://bugs.openjdk.java.net/browse/JDK-8245057 Thank you, Daniil From: "serguei.spit...@oracle.com" Date: Monday, May 11, 2020 at 11:53 AM To: Daniil Titov , serviceability-dev Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools Hi Daniil, It looks pretty good in general. A couple of nits below. http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html +void *cursor; +jbyte argumentTag; +jint argIndex = 0; +jvalue *argument = request->arguments;; . . . void *cursor; jint argIndex = 0; +jbyte argumentTag; jvalue *argument = request->arguments; It is better if the local variables 'cursor' and 'argumentTag' get initialized. There is double semicolon: "arguments;;" http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html 43 static inline jbyte basicType(const char *signature) { It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. It is more safe to run tier5 as well. Thanks, Serguei On 5/9/20 09:29, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, In the grand scheme of things, could servicability use the signature parsing support in HotSpot? Thanks, Roger On 5/9/20 12:29 PM, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, It looks pretty good in general. A couple of nits below. http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html +void *cursor; +jbyte argumentTag; +jint argIndex = 0; +jvalue *argument = request->arguments;; . . . void *cursor; jint argIndex = 0; +jbyte argumentTag; jvalue *argument = request->arguments; It is better if the local variables 'cursor' and 'argumentTag' get initialized. There is double semicolon: "arguments;;" http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html 43 static inline jbyte basicType(const char *signature) { It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. It is more safe to run tier5 as well. Thanks, Serguei On 5/9/20 09:29, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, Overall looks good. Just one minor thing. In ClassTypeImpl.c and ObjectReferenceImpl.c I think the following would be more readable with an if/else: 79 return JNI_FUNC_PTR(env,ExceptionOccurred)(env) ? AGENT_ERROR_JNI_EXCEPTION 80 : JVMTI_ERROR_NONE; Also, have you filed a CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions? thanks, Chris On 5/9/20 9:29 AM, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil