Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule

[Daniel D. Daugherty]

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.

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

Spec wording is very difficult... :-)

Dan


>
> Thanks,
> Serguei
>
>
>
>> /Jesper
>>
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>> David
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Dan
>