Hi Jc,

Thank you for filing this issue!
It should not be hard to fix but will need a CSR as David noted.

Thanks,
Serguei


On 5/14/19 08:13, Jean Christophe Beyler wrote:
Hi Serguei,

For what it's worth, I created https://bugs.openjdk.java.net/browse/JDK-8223881 so that we can hopefully work on it once this and 13 goes through :)
Jc

*From: *[email protected] <mailto:[email protected]> <[email protected] <mailto:[email protected]>>
*Date: *Tue, May 14, 2019 at 3:12 AM
*To: *David Holmes, Jean Christophe Beyler
*Cc: *serviceability-dev

    Hi David,

    Thank you a lot!
    Serguei


    On 5/14/19 00:13, David Holmes wrote:
    > Hi Serguei,
    >
    > For the delay in getting back to this. Everything seems fine to
    me now.
    >
    > Thanks,
    > David
    > -----
    >
    > On 10/05/2019 7:16 pm, [email protected]
    <mailto:[email protected]> wrote:
    >> Hi David,
    >>
    >> I've noticed a minor problem in the jvmti.html diff below:
    >>
    >> 5c5
    >> <         <title>JVM(TM) Tool Interface 11.0.0</title>
    >> ---
    >>  >         <title>JVM(TM) Tool Interface 13.0.0</title>
    >> 30c30
    >> <             <h3>Version 11.0</h3>
    >> ---
    >>  >             <h3>Version 13.0</h3>
    >> 34931c34931
    >> <                 Version: 11.0.0
    >> ---
    >>  >                 Version: 13.0.0
    >>
    >>
    >> There should not be the last difference as this is the version of
    >> last JVMTI spec update:
    >>
    >> *11.0.0*
    >> 7 February 2018     Minor update for new class file NestHost and
    >> NestMembers attributes: - Specify that RedefineClasses and
    >> RetransformClasses are not allowed to change the class file
    NestHost
    >> and NestMembers attributes. - Add new error
    >> JVMTI_ERROR_UNSUPPORTED_REDEFINITION_CLASS_ATTRIBUTE_CHANGED
    that can
    >> be returned by RedefineClasses and RetransformClasses.
    >>
    >>
    >>
    >> I've updated the webrev:
    >>
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.3/
    <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8219023-svc-version.3/>
    >>
    >> The newly updated file is:
    >> || src/hotspot/share/prims/jvmti.xml
    >>
    >> which has this change:
    >>
    >> +<xsl:template name="lastchangeversion">
    >> + <xsl:for-each select="//change">
    >> + <xsl:if test="position() = last()">
    >> + <xsl:value-of select="@version"/>
    >> + </xsl:if>
    >> + </xsl:for-each>
    >> +</xsl:template>
    >> +
    >>   <xsl:template match="changehistory">
    >>       <div class="sep"/>
    >>       <hr class="thick"/>
    >>       <h2>Change History</h2>
    >>       Last update: <xsl:value-of select="@update"/><br/>
    >> - Version: <xsl:call-template name="showversion"/>
    >> + Version: <xsl:call-template name="lastchangeversion"/>
    >>
    >>
    >> New jvmti.html diff is:
    >> 5c5
    >> <         <title>JVM(TM) Tool Interface 11.0.0</title>
    >> ---
    >>  >         <title>JVM(TM) Tool Interface 13.0.0</title>
    >> 30c30
    >> <             <h3>Version 11.0</h3>
    >> ---
    >>  >             <h3>Version 13.0</h3>
    >>
    >>
    >> Thanks,
    >> Serguei
    >>
    >>
    >> On 5/10/19 01:03, [email protected]
    <mailto:[email protected]> wrote:
    >>> Hi David,
    >>>
    >>>
    >>> On 5/9/19 18:51, David Holmes wrote:
    >>>> Hi Serguei,
    >>>>
    >>>> On 10/05/2019 10:32 am, [email protected]
    <mailto:[email protected]> wrote:
    >>>>> I've updated the webrev v2 in place.
    >>>>
    >>>>  make/hotspot/gensrc/GensrcJvmti.gmk
    >>>>
    >>>> You don't need to pass through: -PARAM minorversion
    $(VERSION_INTERIM)
    >>>
    >>> Good catch.
    >>> How did I missed to remove?
    >>>
    >>>
    >>>>
    src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java

    >>>>
    >>>>
    >>>>  57     private static final int minorVersion =
    >>>> Runtime.version().interim();
    >>>>
    >>>> That should be kept at 0.
    >>>
    >>> Okay, fixed.
    >>>
    >>> New webrev is:
    >>>
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.3/
    <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8219023-svc-version.3/>

    >>>
    >>>
    >>>
    >>>>
    >>>> I'd like to see an actual diff of the generated jvmti.h and
    >>>> jvmti.html files as well please. Some of the XSL stuff looks
    odd to
    >>>> me.
    >>>
    >>> The jvmti.h diff:
    >>>
    >>> 2c2
    >>> <  * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All
    >>> rights reserved.
    >>> ---
    >>> >  * Copyright (c) 2002, 2019, Oracle and/or its affiliates. All
    >>> rights reserved.
    >>> 47c47
    >>> <     JVMTI_VERSION = 0x30000000 + (11 * 0x10000) + (0 *
    0x100) + 0
    >>> /* version: 11.0.0 */
    >>> ---
    >>> >     JVMTI_VERSION = 0x30000000 + (13 * 0x10000) + ( 0 *
    0x100) +
    >>> 0  /* version: 13.0.0 */
    >>>
    >>>
    >>>
    >>> The jvmti.html diff:
    >>>
    >>> 5c5
    >>> <         <title>JVM(TM) Tool Interface 11.0.0</title>
    >>> ---
    >>> >         <title>JVM(TM) Tool Interface 13.0.0</title>
    >>> 30c30
    >>> <             <h3>Version 11.0</h3>
    >>> ---
    >>> >             <h3>Version 13.0</h3>
    >>> 34931c34931
    >>> <                 Version: 11.0.0
    >>> ---
    >>> >                 Version: 13.0.0
    >>>
    >>>
    >>>
    >>> Thanks,
    >>> Serguei
    >>>
    >>>>
    >>>> Thanks,
    >>>> David
    >>>>
    >>>>> Thanks,
    >>>>> Serguei
    >>>>>
    >>>>>
    >>>>> On 5/9/19 17:28, [email protected]
    <mailto:[email protected]> wrote:
    >>>>>> David and Jc,
    >>>>>>
    >>>>>> Okay, I'll remove this line now.
    >>>>>> Thank you for your comments.
    >>>>>>
    >>>>>> Let's let Jc to file a separate enhancement on this.
    >>>>>> Then I'll file a CSR and prepare a fix.
    >>>>>>
    >>>>>> I hope, you both are Okay with the rest.
    >>>>>>
    >>>>>> Thanks!
    >>>>>> Serguei
    >>>>>>
    >>>>>>
    >>>>>> On 5/9/19 17:17, Jean Christophe Beyler wrote:
    >>>>>>> Hi Serguei,
    >>>>>>>
    >>>>>>> Adding to the difficulties that David is exposing, this won't
    >>>>>>> work. You need to redo the xls definition because you need
    the
    >>>>>>> #define to be the numeric value directly and not the enum;
    >>>>>>> otherwise it won't work in any usable way at preprocessor
    time
    >>>>>>> sadly.
    >>>>>>>
    >>>>>>> I think it makes sense to just do what you were planning
    to do
    >>>>>>> here without this and I'll file a bug and work out the CSR
    path
    >>>>>>> and review path separately and see what is do-able or not
    then
    >>>>>>> because I think it's too much work now "just for this now" if
    >>>>>>> that makes sense :)
    >>>>>>> Jc
    >>>>>>>
    >>>>>>> *From: *David Holmes <[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>>
    >>>>>>> *Date: *Thu, May 9, 2019 at 5:11 PM
    >>>>>>> *To: *[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>, Jean Christophe Beyler
    >>>>>>> *Cc: *serviceability-dev
    >>>>>>>
    >>>>>>>     On 10/05/2019 9:45 am, [email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>> wrote:
    >>>>>>>     > Hi Jc,
    >>>>>>>     >
    >>>>>>>     > Okay, you convinced me - thanks!
    >>>>>>>     > Added new line into the generated jvmti.h:
    >>>>>>>     >
    >>>>>>>     >    #define JVMTI_VERSION_LATEST JVMTI_VERSION
    >>>>>>>
    >>>>>>>     That requires a CSR as you are expanding the exported
    >>>>>>> interface.
    >>>>>>>
    >>>>>>>     Also you need
    >>>>>>>
    >>>>>>>     #define JVMTI_VERSION_LATEST (JVMTI_VERSION)
    >>>>>>>
    >>>>>>>     as JVMTI_VERSION is itself an expression not a constant.
    >>>>>>> That might
    >>>>>>>     limit the utility of having such a define as you won't be
    >>>>>>> able to
    >>>>>>>     use it
    >>>>>>>     in an ifdef guard to test a value AFAICS.
    >>>>>>>
    >>>>>>>     David
    >>>>>>>
    >>>>>>>     > I hope, it will help in your case.
    >>>>>>>     >
    >>>>>>>     >
    >>>>>>>     > Updated webrev version is:
    >>>>>>>     >
    >>>>>>>
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.2/
    <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8219023-svc-version.2/>

    >>>>>>>
    >>>>>>>     >
    >>>>>>>     >
    >>>>>>>     > This version includes suggestions from David.
    >>>>>>>     >
    >>>>>>>     > Thanks,
    >>>>>>>     > Serguei
    >>>>>>>     >
    >>>>>>>     >
    >>>>>>>     >
    >>>>>>>     > On 5/9/19 14:17, Jean Christophe Beyler wrote:
    >>>>>>>     >> Hi Serguei,
    >>>>>>>     >>
    >>>>>>>     >> Of course I can :)
    >>>>>>>     >>
    >>>>>>>     >> Consider, just randomly of course, the heap
    sampling work
    >>>>>>> that
    >>>>>>>     got
    >>>>>>>     >> added to JVMTI. Now imagine you'd want to test if it is
    >>>>>>>     supported by a
    >>>>>>>     >> given JVMTI version, you would write something like
    this:
    >>>>>>>     >>
    >>>>>>>     >> bool HeapMonitor::Supported(jvmtiEnv *jvmti) {
    >>>>>>>     >>   jvmtiCapabilities caps;
    >>>>>>>     >>   memset(&caps, 0, sizeof(caps));
    >>>>>>>     >>   if (jvmti->GetPotentialCapabilities(&caps) !=
    >>>>>>>     JVMTI_ERROR_NONE) {
    >>>>>>>     >>     LOG(WARNING) << "Failed to get potential
    capabilities,
    >>>>>>>     disabling
    >>>>>>>     >> the heap "
    >>>>>>>     >>  << "sampling monitor";
    >>>>>>>     >>     return false;
    >>>>>>>     >>   }
    >>>>>>>     >>
    >>>>>>>     >>   return caps.can_generate_sampled_object_alloc_events
    >>>>>>>     >>       && caps.can_generate_garbage_collection_events;
    >>>>>>>     >> }
    >>>>>>>     >>
    >>>>>>>     >> Now, the problem is that this code cannot be used
    if you
    >>>>>>>     compile with
    >>>>>>>     >> an older JDK such as JDK8 for example
    >>>>>>>     >> because can_generate_sampled_object_alloc_events
    does not
    >>>>>>>     exist yet
    >>>>>>>     >> for that jvmti.h file.
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >> In a perfect world, we might imagine that we are always
    >>>>>>>     compiling with
    >>>>>>>     >> the latest JVMTI headers but that is not always
    true and,
    >>>>>>>     therefore,
    >>>>>>>     >> to have the code portable, we now have to do:
    >>>>>>>     >>
    >>>>>>>     >> bool HeapMonitor::Supported(jvmtiEnv *jvmti) {
    >>>>>>>     >> #ifdef ENABLE_HEAP_SAMPLING
    >>>>>>>     >>   jvmtiCapabilities caps;
    >>>>>>>     >>   memset(&caps, 0, sizeof(caps));
    >>>>>>>     >>   if (jvmti->GetPotentialCapabilities(&caps) !=
    >>>>>>>     JVMTI_ERROR_NONE) {
    >>>>>>>     >>     LOG(WARNING) << "Failed to get potential
    capabilities,
    >>>>>>>     disabling
    >>>>>>>     >> the heap "
    >>>>>>>     >>  << "sampling monitor";
    >>>>>>>     >>     return false;
    >>>>>>>     >>   }
    >>>>>>>     >>
    >>>>>>>     >>   return caps.can_generate_sampled_object_alloc_events
    >>>>>>>     >>       && caps.can_generate_garbage_collection_events;
    >>>>>>>     >> #else
    >>>>>>>     >>   return false;
    >>>>>>>     >> #endif
    >>>>>>>     >> }
    >>>>>>>     >>
    >>>>>>>     >> Where ENABLE_HEAP_SAMPLING is defined if we did
    compile with
    >>>>>>>     JDK11 and
    >>>>>>>     >> not defined if we compiled with JDK8. I can't use
    >>>>>>> JVMTI_VERSION
    >>>>>>>     >> because I can't use it in an #if because it is not
    an enum.
    >>>>>>>     Were it to
    >>>>>>>     >> be a #define or were I to have a #define I could
    use with a
    >>>>>>>     version
    >>>>>>>     >> number being bumped up, I could write something
    such as:
    >>>>>>>     >>
    >>>>>>>     >> #ifdef JVMTI_VERSION_11
    >>>>>>>     >>
    >>>>>>>     >> or something that looks in the value of the
    JVMTI_VERSION
    >>>>>>> if I
    >>>>>>>     wanted.
    >>>>>>>     >> Right now, I can't even do that!
    >>>>>>>     >>
    >>>>>>>     >> Hopefully this helps understand what I am talking
    about :-),
    >>>>>>>     >> Jc
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >> On Thu, May 9, 2019 at 2:08 PM
    [email protected] <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >> <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>> <[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >> <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>>> wrote:
    >>>>>>>     >>
    >>>>>>>     >>     Hi Jc,
    >>>>>>>     >>
    >>>>>>>     >>     Thank you a lot for review!
    >>>>>>>     >>     Some replies below.
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >>     On 5/9/19 09:10, Jean Christophe Beyler wrote:
    >>>>>>>     >>>     Hi Serguei,
    >>>>>>>     >>>
    >>>>>>>     >>>     FWIW, the change looks good and I think it's a
    good
    >>>>>>> idea
    >>>>>>>     to do.
    >>>>>>>     >>>     However, there is one thorn in our internal agent
    >>>>>>> code is
    >>>>>>>     that
    >>>>>>>     >>>     the JVMTI_VERSION is in an enum. This makes us
    >>>>>>> unable to
    >>>>>>>     #if it
    >>>>>>>     >>>     when adding usages of newer features/methods.
    >>>>>>>     >>>
    >>>>>>>     >>>     This probably could/should be a different webrev
    >>>>>>> (which I
    >>>>>>>     can do
    >>>>>>>     >>>     if you like) but is there any way while you are
    >>>>>>> changing this
    >>>>>>>     >>>     that the enum for JVMTI_VERSION could become a
    set of
    >>>>>>>     #define?
    >>>>>>>     >>>
    >>>>>>>     >>>     So instead of:
    >>>>>>>     >>>     enum {
    >>>>>>>     >>> JVMTI_VERSION_1   = 0x30010000,
    >>>>>>>     >>> JVMTI_VERSION_1_0 = 0x30010000,
    >>>>>>>     >>> JVMTI_VERSION_1_1 = 0x30010100,
    >>>>>>>     >>> JVMTI_VERSION_1_2 = 0x30010200,
    >>>>>>>     >>> JVMTI_VERSION_9   = 0x30090000,
    >>>>>>>     >>> JVMTI_VERSION_11  = 0x300B0000,
    >>>>>>>     >>>
    >>>>>>>     >>> JVMTI_VERSION = 0x30000000 + (11 * 0x10000) + (0 *
    >>>>>>>     0x100) +
    >>>>>>>     >>>     0  /* version: 11.0.0 */
    >>>>>>>     >>>     };
    >>>>>>>     >>>
    >>>>>>>     >>>     We would get:
    >>>>>>>     >>>     #define JVMTI_VERSION_1 0x30010000
    >>>>>>>     >>>     #define JVMTI_VERSION_1_0 0x30010000
    >>>>>>>     >>>     #define JVMTI_VERSION_1_1 = 0x30010100
    >>>>>>>     >>>     #define JVMTI_VERSION_1_2 = 0x30010200
    >>>>>>>     >>>     #define JVMTI_VERSION_9   = 0x30090000
    >>>>>>>     >>>     #define JVMTI_VERSION_11  = 0x300B0000
    >>>>>>>     >>>     #define JVMTI_VERSION (0x30000000 + (11 *
    0x10000) +
    >>>>>>> (0 *
    >>>>>>>     0x100)
    >>>>>>>     >>>     + 0  /* version: 11.0.0 */)
    >>>>>>>     >>
    >>>>>>>     >>     It is interesting concern and suggestion.
    >>>>>>>     >>     I'm not sure if it requires a CSR.
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >>>     I actually don't care about any define of these
    >>>>>>> except for
    >>>>>>>     >>>  JVMTI_VERSION; basically it would be useful so
    that in
    >>>>>>>     our agent
    >>>>>>>     >>>     code we can test the JVMTI_VERSION with #if
    macros to
    >>>>>>>     protect the
    >>>>>>>     >>>     code when new elements show up in future
    versions.
    >>>>>>> So it also
    >>>>>>>     >>>     could be:
    >>>>>>>     >>>     enum {
    >>>>>>>     >>> JVMTI_VERSION_1   = 0x30010000,
    >>>>>>>     >>> JVMTI_VERSION_1_0 = 0x30010000,
    >>>>>>>     >>> JVMTI_VERSION_1_1 = 0x30010100,
    >>>>>>>     >>> JVMTI_VERSION_1_2 = 0x30010200,
    >>>>>>>     >>> JVMTI_VERSION_9   = 0x30090000,
    >>>>>>>     >>> JVMTI_VERSION_11  = 0x300B0000,
    >>>>>>>     >>>
    >>>>>>>     >>> JVMTI_VERSION = 0x30000000 + (11 * 0x10000) + (0 *
    >>>>>>>     0x100) +
    >>>>>>>     >>>     0  /* version: 11.0.0 */
    >>>>>>>     >>>     };
    >>>>>>>     >>>
    >>>>>>>     >>>     #define JVMTI_LATEST_VERSION (0x30000000 + (11 *
    >>>>>>> 0x10000)
    >>>>>>>     + (0 *
    >>>>>>>     >>>     0x100) + 0 /* version: 11.0.0 */)
    >>>>>>>     >>
    >>>>>>>     >>     I is not a problem to implement this one.
    >>>>>>>     >>     But I'm not sure how does this really help in
    your case.
    >>>>>>>     >>     I do not see a point to test the JVMTI_VERSION
    with
    >>>>>>> #if as
    >>>>>>>     it is
    >>>>>>>     >>     always defined.
    >>>>>>>     >>     Could you, please, elaborate a little bit more?
    >>>>>>>     >>
    >>>>>>>     >>     Thanks,
    >>>>>>>     >>     Serguei
    >>>>>>>     >>
    >>>>>>>     >>>     Right now, I have to do weird things where I
    detect the
    >>>>>>>     jvmti.h
    >>>>>>>     >>>     used at compile time to then do -DUSING_JDK11
    for the
    >>>>>>>     agent at
    >>>>>>>     >>>     compile time.
    >>>>>>>     >>>
    >>>>>>>     >>>     Thanks!
    >>>>>>>     >>>     Jc
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>     On Thu, May 9, 2019 at 8:48 AM
    >>>>>>> [email protected] <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >>>  <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>> <[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >>>  <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>>> wrote:
    >>>>>>>     >>>
    >>>>>>>     >>>         I'll try to get rid of VERSION_INTERIM.
    >>>>>>>     >>>         Always using just VERSION_FEATURE.0.0
    should not
    >>>>>>>     create problems
    >>>>>>>     >>>         if we do not change JVMTI spec in
    VERSION_UPDATE.
    >>>>>>>     >>>         I do not see why we would change the JVMTI
    spec
    >>>>>>> in update
    >>>>>>>     >>>  releases.
    >>>>>>>     >>>         But if we do then using VERSION_UPDATE as
    >>>>>>>     microversion would
    >>>>>>>     >>>         be good enough.
    >>>>>>>     >>>
    >>>>>>>     >>>         Thanks!
    >>>>>>>     >>>         Serguei
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>         On 5/9/19 06:13, David Holmes wrote:
    >>>>>>>     >>>         > Hi Serguei,
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > On 9/05/2019 7:09 pm,
    [email protected] <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >>>  <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>> wrote:
    >>>>>>>     >>>         >> Hi David,
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> Thank you a lot for review!
    >>>>>>>     >>>         >> There are some replies below.
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> On 5/8/19 18:42, David Holmes wrote:
    >>>>>>>     >>>  >>> Hi Serguei,
    >>>>>>>     >>>  >>>
    >>>>>>>     >>>  >>> On 9/05/2019 8:57 am,
    >>>>>>> [email protected] <mailto:[email protected]>
    >>>>>>> <mailto:[email protected]
    <mailto:[email protected]>>
    >>>>>>>     >>>  <mailto:[email protected]
    <mailto:[email protected]>
    >>>>>>>   �� <mailto:[email protected]
    <mailto:[email protected]>>> wrote:
    >>>>>>>     >>>  >>>> Please, review a fix for the task:
    >>>>>>>     >>>  >>>>
    >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8219023
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>> Webrev:
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>
    >>>>>>>
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8219023-svc-version.1/
    <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8219023-svc-version.1/>

    >>>>>>>
    >>>>>>>     >>>
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>> Summary:
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>>   By design as we have never bumped the JVMTI
    >>>>>>>     version unless
    >>>>>>>     >>>  >>>>   there were spec changes for that release.
    >>>>>>> Now
    >>>>>>>     we want
    >>>>>>>     >>>         to sync
    >>>>>>>     >>>  >>>>   the JVMTI version with the JDK version
    >>>>>>>     regardless of
    >>>>>>>     >>>         whether
    >>>>>>>     >>>  >>>>   or not spec changes have occurred in that
    >>>>>>> release.
    >>>>>>>     >>>  >>>>   Also, we want it automatically set by the
    >>>>>>>     build system
    >>>>>>>     >>>         so that
    >>>>>>>     >>>  >>>>   no manual updates are needed for each
    >>>>>>> release.
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>>   The jvmti.h and jvmti.html (JVMTI spec) are
    >>>>>>>     generated from
    >>>>>>>     >>>  >>>>   the jvmti.xsl with the XSLT scripts now. So,
    >>>>>>>     the fix
    >>>>>>>     >>>         removes
    >>>>>>>     >>>  >>>>   hard coded major, minor and micro versions
    >>>>>>>     from the
    >>>>>>>     >>>  jvmti.xml
    >>>>>>>     >>>  >>>>   and passes major and minor parameters
    >>>>>>> with the
    >>>>>>>     -PARAMETER
    >>>>>>>     >>>  >>>>   to the XSL transformation.
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>>   Another part of the fix is in the JDI
    >>>>>>> which starts
    >>>>>>>     >>>         using JDK
    >>>>>>>     >>>  >>>>   versions now instead of maintaining its own,
    >>>>>>>     and in
    >>>>>>>     >>>         the JDWP
    >>>>>>>     >>>  >>>>   agent which is using the JVMTI versions
    >>>>>>>     instead of its
    >>>>>>>     >>>         own.
    >>>>>>>     >>>  >>>
    >>>>>>>     >>>  >>> This all seems reasonable (though I'm no
    >>>>>>> expert on
    >>>>>>>     >>>         working with XSL
    >>>>>>>     >>>  >>> etc).
    >>>>>>>     >>>  >>>
    >>>>>>>     >>>  >>> One thing I am unclear of is why you bother
    >>>>>>> with
    >>>>>>>     using
    >>>>>>>     >>>  >>> VERSION_INTERIM when the actual version check
    >>>>>>>     will only
    >>>>>>>     >>>         consider
    >>>>>>>     >>>  >>> VERSION_FEATURE (aka major). Couldn't you just
    >>>>>>>     leave the
    >>>>>>>     >>>         "minor"
    >>>>>>>     >>>  >>> part 0 the same as the "micro" part?
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> This is right question to ask.
    >>>>>>>     >>>         >> I was two-folded on this.
    >>>>>>>     >>>         >> But finally decided to maintain minor
    version
    >>>>>>> (aka
    >>>>>>>     >>>  VERSION_INTERIM).
    >>>>>>>     >>>         >> Then the JVMTI and debugger version will
    >>>>>>> match the
    >>>>>>>     VM and
    >>>>>>>     >>>         JDK version
    >>>>>>>     >>>         >> for update releases.
    >>>>>>>     >>>         >> If understand it correctly, we are still
    >>>>>>> going to have
    >>>>>>>     >>>  major.minor
    >>>>>>>     >>>         >> versions.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > Not really. What we have now are things
    like
    >>>>>>> 11.0.3 and
    >>>>>>>     >>>         12.0.1 - only
    >>>>>>>     >>>         > using the first and third parts. The
    full 4 part
    >>>>>>>     version
    >>>>>>>     >>>         string is:
    >>>>>>>     >>>         >
    >>>>>>>     >>>         >
    >>>>>>>     >>>
    >>>>>>>
     $VERSION_FEATURE.$VERSION_INTERIM.$VERSION_UPDATE.$VERSION_PATCH
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > and we typically only update
    version_feature and
    >>>>>>>     >>>  version_update.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > https://openjdk.java.net/jeps/322
    >>>>>>>     >>>         >
    >>>>>>>     >>>         >> Also, the JVMTI GetVersionNumberspec still
    >>>>>>> tells about
    >>>>>>>     >>>         both minor and
    >>>>>>>     >>>         >> micro versions.
    >>>>>>>     >>>         >> It also defines special constants for
    >>>>>>>     corresponding masks
    >>>>>>>     >>>         and shifts:
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >>     Version Masks
    >>>>>>>     >>>         >>     Constant Value Description
    >>>>>>>     >>>         >> |JVMTI_VERSION_MASK_INTERFACE_TYPE|
    0x70000000
    >>>>>>>     >>>         Mask to
    >>>>>>>     >>>         >> extract
    >>>>>>>     >>>         >>     interface type. The value of the
    version
    >>>>>>>     returned by
    >>>>>>>     >>>         this function
    >>>>>>>     >>>         >>     masked with
    >>>>>>> |JVMTI_VERSION_MASK_INTERFACE_TYPE| is always
    >>>>>>>     >>>         >> |JVMTI_VERSION_INTERFACE_JVMTI| since
    this is
    >>>>>>> a JVMTI
    >>>>>>>     >>>  function.
    >>>>>>>     >>>         >> |JVMTI_VERSION_MASK_MAJOR| 0x0FFF0000
    >>>>>>> Mask to
    >>>>>>>     >>>         extract major
    >>>>>>>     >>>         >>     version number.
    >>>>>>>     >>>         >> |JVMTI_VERSION_MASK_MINOR| 0x0000FF00
    >>>>>>> Mask to
    >>>>>>>     >>>         extract minor
    >>>>>>>     >>>         >>     version number.
    >>>>>>>     >>>         >> |JVMTI_VERSION_MASK_MICRO| 0x000000FF
    >>>>>>> Mask to
    >>>>>>>     >>>         extract micro
    >>>>>>>     >>>         >>     version number.
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >>     Version Shifts
    >>>>>>>     >>>         >>     Constant Value Description
    >>>>>>>     >>>         >> |JVMTI_VERSION_SHIFT_MAJOR|     16 Shift to
    >>>>>>>     extract major
    >>>>>>>     >>>         >> version number.
    >>>>>>>     >>>         >> |JVMTI_VERSION_SHIFT_MINOR|     8 Shift to
    >>>>>>> extract
    >>>>>>>     minor
    >>>>>>>     >>>         >> version number.
    >>>>>>>     >>>         >> |JVMTI_VERSION_SHIFT_MICRO|     0 Shift to
    >>>>>>> extract
    >>>>>>>     micro
    >>>>>>>     >>>         >> version number.
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> This is link to the spec:
    >>>>>>>     >>>         >>
    >>>>>>>     >>>
    >>>>>>>
    
https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#GetVersionNumber

    >>>>>>>
    >>>>>>>     >>>
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> It seems, changing (and/or deprecating)
    this
    >>>>>>> will give
    >>>>>>>     >>>         more problems
    >>>>>>>     >>>         >> than benefits.
    >>>>>>>     >>>         >> It is better to remain compatible with
    previous
    >>>>>>>     releases.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > This is a problem that was flagged when
    the new
    >>>>>>>     versioning
    >>>>>>>     >>>         scheme was
    >>>>>>>     >>>         > introduced but I'm guessing nothing was
    actually
    >>>>>>>     done about
    >>>>>>>     >>>         it. They
    >>>>>>>     >>>         > are not really compatible beyond the
    >>>>>>> major/feature
    >>>>>>>     part.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > If we only update the spec version with the
    >>>>>>> feature
    >>>>>>>     version
    >>>>>>>     >>>         then all
    >>>>>>>     >>>         > versions will have the form N.0.0. However
    >>>>>>> your changes
    >>>>>>>     >>>         will also
    >>>>>>>     >>>         > update if we happen to use a
    VERSION_INTERIM
    >>>>>>> for some
    >>>>>>>     >>>         reason - though
    >>>>>>>     >>>         > the version check will ignore that
    anyway. I'm
    >>>>>>> not
    >>>>>>>     really
    >>>>>>>     >>>         seeing the
    >>>>>>>     >>>         > point in having that happen.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > Maybe we do need to define a new version
    API that
    >>>>>>>     maps to
    >>>>>>>     >>>         the new
    >>>>>>>     >>>         > versioning scheme of OpenJDK ? But if we
    did
    >>>>>>> that we'd
    >>>>>>>     >>>         still have to
    >>>>>>>     >>>         > support the legacy mapping and I'd still
    advocate
    >>>>>>>     simply using
    >>>>>>>     >>>         > VERSION_FEATURE.0.0.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > It's tricky.
    >>>>>>>     >>>         >
    >>>>>>>     >>>         > David
    >>>>>>>     >>>         > -----
    >>>>>>>     >>>         >
    >>>>>>>     >>>  >>> For the record I considered whether this
    >>>>>>> needs a CSR
    >>>>>>>     >>>         request and
    >>>>>>>     >>>  >>> concluded it did not as it doesn't involve
    >>>>>>>     changing any
    >>>>>>>     >>>         actual
    >>>>>>>     >>>  >>> specifications.
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> Okay, thanks.
    >>>>>>>     >>>         >> I considered it too, made the same
    conclusion
    >>>>>>> but
    >>>>>>>     still
    >>>>>>>     >>>         have some
    >>>>>>>     >>>         >> doubt. :)
    >>>>>>>     >>>         >>
    >>>>>>>     >>>         >> Thanks!
    >>>>>>>     >>>         >> Serguei
    >>>>>>>     >>>         >>
    >>>>>>>     >>>  >>>
    >>>>>>>     >>>  >>> Thanks,
    >>>>>>>     >>>  >>> David
    >>>>>>>     >>>  >>>
    >>>>>>>     >>>  >>>> Testing:
    >>>>>>>     >>>  >>>>   Generated docs and jvmti.h and checked the
    >>>>>>>     versions
    >>>>>>>     >>>         are correct.
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>> One could ask if we have to use same or
    >>>>>>> similar
    >>>>>>>     approach for
    >>>>>>>     >>>  >>>> other API's and tools, like JNI, JMX and so
    >>>>>>> on.
    >>>>>>>     >>>  >>>> But these are not areas of my expertise or
    >>>>>>>     responsibility.
    >>>>>>>     >>>  >>>> It just feels like there is some room for
    >>>>>>>     unification here.
    >>>>>>>     >>>  >>>>
    >>>>>>>     >>>  >>>> Thanks,
    >>>>>>>     >>>  >>>> Serguei
    >>>>>>>     >>>         >>
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>
    >>>>>>>     >>>     --
    >>>>>>>     >>>
    >>>>>>>     >>>     Thanks,
    >>>>>>>     >>>     Jc
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >>
    >>>>>>>     >> --
    >>>>>>>     >>
    >>>>>>>     >> Thanks,
    >>>>>>>     >> Jc
    >>>>>>>     >
    >>>>>>>
    >>>>>>>
    >>>>>>>
    >>>>>>> --
    >>>>>>>
    >>>>>>> Thanks,
    >>>>>>> Jc
    >>>>>>
    >>>>>
    >>>
    >>



--

Thanks,
Jc

Reply via email to