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]>>
*Date: *Thu, May 9, 2019 at 5:11 PM
*To: *[email protected]
<mailto:[email protected]>, Jean Christophe Beyler
*Cc: *serviceability-dev
On 10/05/2019 9:45 am, [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/
>
>
> 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]>> <[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]>> <[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]>> 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]>> 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
