|
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
On 10/05/2019 9:45 am, [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]>
<[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]>
<[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]>
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]>
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
>
--
|
