RFR (S): 8219023: Investigate syncing JVMTI spec version with JDK version
David Holmes
david.holmes at oracle.com
Fri May 10 01:51:52 UTC 2019
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)
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.
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.
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