Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools

2020-09-08 Thread Egor Ushakov 

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

2020-05-19 Thread Daniil Titov 
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

2020-05-18 Thread Chris Plummer 

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

2020-05-14 Thread serguei.spit...@oracle.com 

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

2020-05-14 Thread Daniil Titov 
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

2020-05-11 Thread Roger Riggs 

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

2020-05-11 Thread serguei.spit...@oracle.com 

  
  
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

2020-05-11 Thread Chris Plummer 

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

RFR: 8241080: Consolidate signature parsing code in serviceability tools

2020-05-09 Thread Daniil Titov 
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