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

[serguei.spitsyn at oracle.com]

On 6/28/16 19:30, David Holmes wrote:
> 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.

Agreed.
It is a little bit confusing but users seems to be Ok with it.
So that there is no motivation to improve it as it would touch many 
fragments.

Thanks,
Serguei


>
> Thanks,
> David
>
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> David
>>>
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>>
>>>>> Dan
>>>>
>>