RFR (S): 8219023: Investigate syncing JVMTI spec version with JDK version
Jean Christophe Beyler
jcbeyler at google.com
Fri May 10 00:17:19 UTC 2019
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>
*Date: *Thu, May 9, 2019 at 5:11 PM
*To: *serguei.spitsyn at oracle.com, Jean Christophe Beyler
*Cc: *serviceability-dev
On 10/05/2019 9:45 am, 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>> 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>> 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> 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> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190509/ddfa6c27/attachment-0001.html>
More information about the serviceability-dev
mailing list