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, [email protected] wrote:
Hi
David,
On 5/9/19 18:51, David Holmes wrote:
Hi Serguei,
On 10/05/2019 10:32 am, [email protected] 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, [email protected] 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 <[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
|
