[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2016-06-30 3:34:09
Message-ID: 57749331.2030701 () oracle ! com
[Download RAW message or body]
On 6/29/16 19:07, Daniel D. Daugherty wrote:
> On 6/29/16 6:21 PM, serguei.spitsyn@oracle.com wrote:
>> Hi Dan,
>>
>> On 6/29/16 07:59, Daniel D. Daugherty wrote:
>>> On 6/29/16 2:53 AM, serguei.spitsyn@oracle.com wrote:
>>>> On 6/29/16 01:45, Jesper Wilhelmsson wrote:
>>>>> 29 juni 2016 kl. 04:30 skrev David Holmes <david.holmes@oracle.com>:
>>>>>>> On 29/06/2016 12:09 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>> On 6/28/16 18:44, David Holmes wrote:
>>>>>>>>> On 29/06/2016 7:09 AM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>> On 6/28/16 14:02, Daniel D. Daugherty wrote:
>>>>>>>>>>> On 6/28/16 2:11 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>>>> On 6/28/16 11:19, Daniel D. Daugherty wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I'll have to check the upper layers of this API,
>>>>>>>>>>>> but if an
>>>>>>>>>>>> agent can pass a bad 'class_loader' parameter and
>>>>>>>>>>>> get this
>>>>>>>>>>>> assert() to fire, then that's not good. Hopefully a
>>>>>>>>>>>> bad
>>>>>>>>>>>> 'class_loader' parameter is caught at a higher layer.
>>>>>>>>>>> Not sure, what do you mean.
>>>>>>>>>>> Do you mean the generated JVMTI upper layer or the
>>>>>>>>>>> JvmtiEnv::GetNamedModule?
>>>>>>>>>>> Probably, the generated code.
>>>>>>>>>> I did mean the generated layer.
>>>>>>>>> Ok, thanks.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> Update: Yes, passing a non-NULL jobject as the
>>>>>>>>>>>> class_loader
>>>>>>>>>>>> parameter
>>>>>>>>>>>> when the jobject does not refer to a "class
>>>>>>>>>>>> loader" is
>>>>>>>>>>>> caught
>>>>>>>>>>>> at the upper layer.
>>>>>>>>>>> The upper layer does not check that it is a class loader,
>>>>>>>>>>> just for
>>>>>>>>>>> non-NULL.
>>>>>>>>>>> I think, it is good to have an assert here to double-checks the
>>>>>>>>>>> pre-conditions as other caller can be added later.
>>>>>>>>>>> But I'm Ok to get rid of it if you suggest.
>>>>>>>>>> Please keep the asserts. Basically I was mumbling to myself to
>>>>>>>>>> make sure that the asserts could not be reached by user code
>>>>>>>>>> and the "Update:" was to indicate that I did do.
>>>>>>>>> Ok, thanks.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> src/share/vm/prims/jvmti.xml
>>>>>>>>>>>> L6550: <param id="module_ptr">
>>>>>>>>>>>> L6551: <outptr><jobject/></outptr>
>>>>>>>>>>>> L6552: <description>
>>>>>>>>>>>> L6553: On return, points to a
>>>>>>>>>>>> <code>java.lang.reflect.Module</code> object.
>>>>>>>>>>>> L6554: </description>
>>>>>>>>>>>> L6555: </param>
>>>>>>>>>>>>
>>>>>>>>>>>> The above wording doesn't allow for module_ptr to
>>>>>>>>>>>> be NULL
>>>>>>>>>>>> which
>>>>>>>>>>>> is a mismatch with the description.
>>>>>>>>>>> I disagree (or maybe I got it incorrectly).
>>>>>>>>>>> Pointing to NULL and be NULL is different.
>>>>>>>>>>> It is not allowed for the module_ptr to be NULL but Ok to
>>>>>>>>>>> pint to
>>>>>>>>>>> NULL on return.
>>>>>>>>>> I think the description needs to be:
>>>>>>>>>>
>>>>>>>>>> On return, points to a
>>>>>>>>>> <code>java.lang.reflect.Module</code> object
>>>>>>>>>> or points to a <code>NULL</code>.
>>>>>>>>> Agreed, fixed.
>>>>>>>> Disagree. You would say that a pointer is NULL, not that it
>>>>>>>> points to
>>>>>>>> a NULL.
>>>>>>> Why are you disagree?
>>>>>>> The module_ptr is an out argument, it is not allowed to be NULL.
>>>>>>> But the returning value by this pointer can be NULL.
>>>>>> As per later email I see this terminology already exists so is
>>>>>> fine, but I find it confusing to say "points to a NULL" - a NULL
>>>>>> is not an entity. If "foo points to a NULL" that implies to me
>>>>>> "foo == &NULL;" which is nonsense - foo == NULL, and if foo==NULL
>>>>>> then foo points to nothing. But the "pointer to a pointer" aspect
>>>>>> here does confuse things.
>>>>> I would prefer if it said "points to a NULL pointer", or
>>>>> NULL-pointer. I'm not sure what would be the more correct way to
>>>>> write it. Anyway, a NULL pointer is an entity :-)
>>>> Jesper,
>>>>
>>>> Thank you for the comment.
>>>> In fact, I've just used a pattern that is already present in the
>>>> JVMTI spec.
>>>> Objections to the existing pattern are minor, so it is better to be
>>>> more conservative here.
>>>
>>> Perhaps we can use the wording in this JVM/TI function as a model:
>>>
>>> http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#GetCurrentContendedMonitor
>>>
>>>
>>> This function takes a "jobject *" as a parameter and returns a
>>> monitor pointer via that parameter. Pretty similar to what we're
>>> discussing...
>>>
>>> So here's the existing wording example:
>>>
>>> Name Type Description
>>> =========== ======== ===========
>>> monitor_ptr jobject* On return, filled with the current contended
>>> monitor,
>>> or NULL if there is none.
>>>
>>> Agent passes a pointer to a jobject. On
>>> return, the
>>> jobject has been set. The object returned by
>>> monitor_ptr
>>> is a JNI local reference and must be managed.
>>>
>>> So for the new function:
>>>
>>> module_ptr jobject* On return, filled with a
>>> <code>java.lang.reflect.Module</code>
>>> object or NULL if there is none.
>>>
>>> This "filled with" style is used for return parameters in
>>> ~13 places in the JVM/TI spec.
>>
>> Thank you for pointing to the example.
>> This discussion deviated to the question what the JVMTI pattern is
>> better to copy as a best practice.
>>
>>
>>>
>>> Of course, now that I've found the GetCurrentContendedMonitor
>>> example, That second paragraph or something like it is also
>>> going to be needed with our new function...
>>
>> Just wanted to note that this paragraph is not strictly necessary as
>> it is already covered in the common part of the spec:
>> http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#refs
>>
>> However, the second sentence is auto-generated from the jvmti.xml:
>> "Agent passes a pointer to a |jobject|. On return, the |jobject|
>> has been set.
>> The object returned by |module_ptr| is a JNI local reference and must
>> be managed."
>>
>> Please, find the specdiff in the attachment.
>
> The specdiff looks good.
>
> There is one strange sentence:
>
> > If class_loader is NULL, the bootstrap loader.
>
> Perhaps "the bootstrap loader is assumed."?
>
> Your call.
Interesting that a part of this sentence is auto-generated.
Fixed it, thanks!
Thanks,
Serguei
>
> Dan
>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> Spec wording is very difficult... :-)
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>
>>>>> /Jesper
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Dan
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 6/29/16 19:07, Daniel D. Daugherty
wrote:<br>
</div>
<blockquote
cite="mid:263e1aac-5876-111b-0ee8-7ea5a3f9d883@oracle.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
On 6/29/16 6:21 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
<blockquote cite="mid:5774661A.3080500@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Dan,<br>
<br>
On 6/29/16 07:59, Daniel D. Daugherty wrote:<br>
</div>
<blockquote
cite="mid:4edd4611-6e6b-d61d-22c4-c099f02db2d8@oracle.com"
type="cite">On 6/29/16 2:53 AM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote: <br>
<blockquote type="cite">On 6/29/16 01:45, Jesper Wilhelmsson
wrote: <br>
<blockquote type="cite">29 juni 2016 kl. 04:30 skrev David
Holmes <a moz-do-not-send="true"
class="moz-txt-link-rfc2396E"
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a>:
<br>
<blockquote type="cite">
<blockquote type="cite">On 29/06/2016 12:09 PM, <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"><a \
class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a></a> wrote: \
<br> <blockquote type="cite">On 6/28/16 18:44, David Holmes
wrote: <br>
<blockquote type="cite">On 29/06/2016 7:09 AM, <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"><a \
class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a></a> wrote: \
<br> <blockquote type="cite">On 6/28/16 14:02, Daniel
D. Daugherty wrote: <br>
<blockquote type="cite">On 6/28/16 2:11 PM, <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"><a \
class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a></a> wrote: \
<br> <blockquote type="cite">On 6/28/16 11:19,
Daniel D. Daugherty wrote: <br>
<br>
<br>
I'll have to check the upper layers
of this API, but if an <br>
agent can pass a bad 'class_loader'
parameter and get this <br>
assert() to fire, then that's not
good. Hopefully a bad <br>
'class_loader' parameter is caught
at a higher layer. <br>
</blockquote>
Not sure, what do you mean. <br>
Do you mean the generated JVMTI upper layer or
the <br>
JvmtiEnv::GetNamedModule? <br>
Probably, the generated code. <br>
</blockquote>
I did mean the generated layer. <br>
</blockquote>
Ok, thanks. <br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">
<blockquote type="cite"> Update: Yes,
passing a non-NULL jobject as the
class_loader <br>
parameter <br>
when the jobject does not refer
to a "class loader" is <br>
caught <br>
at the upper layer. <br>
</blockquote>
The upper layer does not check that it is a
class loader, just for <br>
non-NULL. <br>
I think, it is good to have an assert here to
double-checks the <br>
pre-conditions as other caller can be added
later. <br>
But I'm Ok to get rid of it if you suggest. <br>
</blockquote>
Please keep the asserts. Basically I was
mumbling to myself to <br>
make sure that the asserts could not be reached
by user code <br>
and the "Update:" was to indicate that I did do.
<br>
</blockquote>
Ok, thanks. <br>
<br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">
<blockquote type="cite">src/share/vm/prims/jvmti.xml
<br>
L6550: <param
id="module_ptr"> <br>
L6551:
<outptr><jobject/></outptr>
<br>
L6552: <description> <br>
L6553: On return, points to
a <br>
<code>java.lang.reflect.Module</code>
object. <br>
L6554: </description> <br>
L6555: </param> <br>
<br>
The above wording doesn't allow for
module_ptr to be NULL <br>
which <br>
is a mismatch with the description.
<br>
</blockquote>
I disagree (or maybe I got it incorrectly). <br>
Pointing to NULL and be NULL is different. <br>
It is not allowed for the module_ptr to be
NULL but Ok to pint to <br>
NULL on return. <br>
</blockquote>
I think the description needs to be: <br>
<br>
On return, points to a
<code>java.lang.reflect.Module</code>
object <br>
or points to a
<code>NULL</code>. <br>
</blockquote>
Agreed, fixed. <br>
</blockquote>
Disagree. You would say that a pointer is NULL, not
that it points to <br>
a NULL. <br>
</blockquote>
Why are you disagree? <br>
The module_ptr is an out argument, it is not allowed
to be NULL. <br>
But the returning value by this pointer can be NULL. <br>
</blockquote>
As per later email I see this terminology already exists
so is fine, but I find it confusing to say "points to a
NULL" - a NULL is not an entity. If "foo points to a
NULL" that implies to me "foo == &NULL;" which is
nonsense - foo == NULL, and if foo==NULL then foo points
to nothing. But the "pointer to a pointer" aspect here
does confuse things. <br>
</blockquote>
I would prefer if it said "points to a NULL pointer", or
NULL-pointer. I'm not sure what would be the more correct
way to write it. Anyway, a NULL pointer is an entity :-) <br>
</blockquote>
Jesper, <br>
<br>
Thank you for the comment. <br>
In fact, I've just used a pattern that is already present in
the JVMTI spec. <br>
Objections to the existing pattern are minor, so it is
better to be more conservative here. <br>
</blockquote>
<br>
Perhaps we can use the wording in this JVM/TI function as a
model: <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#GetCurrentContend \
edMonitor">http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#GetCurrentContendedMonitor</a>
<br>
<br>
This function takes a "jobject *" as a parameter and returns a
<br>
monitor pointer via that parameter. Pretty similar to what
we're <br>
discussing... <br>
<br>
So here's the existing wording example: <br>
<br>
Name Type Description <br>
=========== ======== =========== <br>
monitor_ptr jobject* On return, filled with the current
contended monitor, <br>
or NULL if there is none. <br>
<br>
Agent passes a pointer to a jobject. On
return, the <br>
jobject has been set. The object
returned by monitor_ptr <br>
is a JNI local reference and must be
managed. <br>
<br>
So for the new function: <br>
<br>
module_ptr jobject* On return, filled with a
<code>java.lang.reflect.Module</code> <br>
object or NULL if there is none. <br>
<br>
This "filled with" style is used for return parameters in <br>
~13 places in the JVM/TI spec.<br>
</blockquote>
<br>
Thank you for pointing to the example.<br>
This discussion deviated to the question what the JVMTI pattern
is better to copy as a best practice.<br>
<br>
<br>
<blockquote
cite="mid:4edd4611-6e6b-d61d-22c4-c099f02db2d8@oracle.com"
type="cite"> <br>
Of course, now that I've found the GetCurrentContendedMonitor
<br>
example, That second paragraph or something like it is also <br>
going to be needed with our new function... <br>
</blockquote>
<br>
Just wanted to note that this paragraph is not strictly
necessary as<br>
it is already covered in the common part of the spec:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#refs">http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#refs</a><br>
<br>
However, the second sentence is auto-generated from the
jvmti.xml:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<span class="diff-html-added"> "Agent</span> <span
class="diff-html-added">passes</span> <span
class="diff-html-added">a</span> <span
class="diff-html-added">pointer</span> <span
class="diff-html-added">to</span> <span
class="diff-html-added">a</span> <code><span
class="diff-html-added">jobject</span></code><span
class="diff-html-added">.</span> <span
class="diff-html-added">On</span> <span
class="diff-html-added">return,</span> <span
class="diff-html-added">the</span> <code><span
class="diff-html-added">jobject</span></code> <span
class="diff-html-added">has</span> <span
class="diff-html-added">been</span> <span
class="diff-html-added">set.</span><br>
<span class="diff-html-added">The</span> <span
class="diff-html-added">object</span> <span
class="diff-html-added">returned</span> <span
class="diff-html-added">by</span> <code><span
class="diff-html-added">module</span>_ptr</code> <span
class="diff-html-added">is</span> <span
class="diff-html-added">a</span> <span
class="diff-html-added">JNI</span> <span
class="diff-html-added">local</span> <span
class="diff-html-added">reference</span> <span
class="diff-html-added">and</span> <span
class="diff-html-added">must</span> <span
class="diff-html-added">be</span> managed."<br>
<br>
Please, find the specdiff in the attachment.<br>
</blockquote>
<tt> <br>
The specdiff looks good.<br>
<br>
There is one strange sentence:<br>
<br>
> If class_loader is NULL, the bootstrap loader.<br>
<br>
Perhaps "the bootstrap loader is assumed."?<br>
<br>
Your call.<br>
</tt></blockquote>
<br>
Interesting that a part of this sentence is auto-generated.<br>
Fixed it, thanks!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote
cite="mid:263e1aac-5876-111b-0ee8-7ea5a3f9d883@oracle.com"
type="cite"><tt> <br>
Dan<br>
<br>
</tt>
<blockquote cite="mid:5774661A.3080500@oracle.com" type="cite"> <br>
Thanks,<br>
Serguei<br>
<br>
<blockquote
cite="mid:4edd4611-6e6b-d61d-22c4-c099f02db2d8@oracle.com"
type="cite"> <br>
Spec wording is very difficult... :-) <br>
<br>
Dan <br>
<br>
<br>
<blockquote type="cite"> <br>
Thanks, <br>
Serguei <br>
<br>
<br>
<br>
<blockquote type="cite">/Jesper <br>
<br>
<br>
<blockquote type="cite">Thanks, <br>
David <br>
<br>
<blockquote type="cite">Thanks, <br>
Serguei <br>
<br>
<br>
<blockquote type="cite">David <br>
<br>
<br>
<blockquote type="cite">Thanks, <br>
Serguei <br>
<br>
<br>
<blockquote type="cite"> <br>
Dan <br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic