RFR: 8241080: Consolidate signature parsing code in serviceability tools

[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" <chris.plummer at oracle.com> 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.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>      > Date: Monday, May 11, 2020 at 11:53 AM
>      > To: Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev <serviceability-dev at openjdk.java.net>
>      > 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