|
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
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] 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] 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
>>
--
|
