RFR (S): 8219023: Investigate syncing JVMTI spec version with JDK version
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri May 10 08:03:36 UTC 2019
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>>
>>>> *Date: *Thu, May 9, 2019 at 5:11 PM
>>>> *To: *serguei.spitsyn at oracle.com
>>>> <mailto: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> 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>
>>>> >> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto:serguei.spitsyn at oracle.com>> <serguei.spitsyn at oracle.com
>>>> <mailto:serguei.spitsyn at oracle.com>
>>>> >> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto: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>
>>>> >>> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto:serguei.spitsyn at oracle.com>> <serguei.spitsyn at oracle.com
>>>> <mailto:serguei.spitsyn at oracle.com>
>>>> >>> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto: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>
>>>> >>> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto: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>
>>>> >>> <mailto:serguei.spitsyn at oracle.com
>>>> <mailto: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
>>>
>>
More information about the serviceability-dev
mailing list