8241080: Consolidate signature parsing code in serviceability tools

[Daniil Titov]

Hi Egor,

Thank you for finding this problem. I created a new Jira issue [1] for that.

[1] https://bugs.openjdk.java.net/browse/JDK-8252933

Best regards,
Daniil

On 9/8/20, 8:31 AM, "Egor Ushakov" <egor.ushakov at jetbrains.com> wrote:

    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
    https://urldefense.com/v3/__http://www.jetbrains.com__;!!GqivPVa7Brio!JOhCc9k54KkUPFPbiviF1MS9YfERc-yqYJLt1n1eVlGWDpapPTdxtFv6f9a6PBgP129w$ 
    The Drive to Develop