RFR (S): 8219023: Investigate syncing JVMTI spec version with JDK version
Jean Christophe Beyler
jcbeyler at google.com
Fri May 10 19:02:37 UTC 2019
Hi Serguei,
Looks good to me too :)
Jc
*From: *serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
*Date: *Fri, May 10, 2019 at 11:44 AM
*To: *Chris Plummer, David Holmes, Jean Christophe Beyler
*Cc: *serviceability-dev
Thanks a lot, Chris!
> Serguei
>
>
> On 5/10/19 11:43, Chris Plummer wrote:
>
> Hi Serguei,
>
> I won't pretend to understand your xsl changes for lastchangeversion, but
> it seems to be producing the proper result. I've re-looked at all the other
> changes too, and looks good to me.
>
> thanks,
>
> Chris
>
> On 5/10/19 2:16 AM, serguei.spitsyn at oracle.com 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/
>
> 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, serguei.spitsyn at oracle.com wrote:
>
> Hi David,
>
>
> On 5/9/19 18:51, David Holmes wrote:
>
> Hi Serguei,
>
> On 10/05/2019 10:32 am, serguei.spitsyn at oracle.com 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/
>
>
>
> 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, serguei.spitsyn at oracle.com 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 <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com> <david.holmes at oracle.com>>
> *Date: *Thu, May 9, 2019 at 5:11 PM
> *To: *serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com>
> <serguei.spitsyn at oracle.com>, Jean Christophe Beyler
> *Cc: *serviceability-dev
>
> On 10/05/2019 9:45 am, serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> 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/
> >
> >
> > 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 serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >> <mailto:serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>> <
> serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >> <mailto:serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>>>
> 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 serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >>> <mailto:serguei.spitsyn at oracle.com
> <serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>> <
> serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >>> <mailto:serguei.spitsyn at oracle.com
> <serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>>>
> 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, serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >>> <mailto:serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>>
> 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, serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
> >>> <mailto:serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
> �� <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>>
> 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/
> >>>
> >>> >>>>
> >>> >>>>
> >>> >>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190510/431d1338/attachment-0001.html>
More information about the serviceability-dev
mailing list