Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jun 30 03:34:09 UTC 2016
On 6/29/16 19:07, Daniel D. Daugherty wrote:
> On 6/29/16 6:21 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Dan,
>>
>> On 6/29/16 07:59, Daniel D. Daugherty wrote:
>>> On 6/29/16 2:53 AM, serguei.spitsyn at oracle.com wrote:
>>>> On 6/29/16 01:45, Jesper Wilhelmsson wrote:
>>>>> 29 juni 2016 kl. 04:30 skrev David Holmes <david.holmes at oracle.com>:
>>>>>>> On 29/06/2016 12:09 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> On 6/28/16 18:44, David Holmes wrote:
>>>>>>>>> On 29/06/2016 7:09 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> On 6/28/16 14:02, Daniel D. Daugherty wrote:
>>>>>>>>>>> On 6/28/16 2:11 PM, serguei.spitsyn at 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
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160629/5c58802a/attachment-0001.html>
More information about the serviceability-dev
mailing list