|
Hi Jc,
Thank you a lot!
Serguei
On 5/10/19 12:02, Jean Christophe Beyler wrote:
Hi Serguei,
Looks good to me too :)
Jc
Thanks
a lot, Chris!
Serguei
On 5/10/19 11:43, Chris Plummer wrote:
Hi
Serguei,
I
won't pretend to understand your xsl changes for
lastchangeversion, but it seems to be producing the
proper result. I've re-looked at all the other changes
too, and looks good to me.
thanks,
Chris
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
--
|
