Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[serguei.spitsyn at oracle.com]

On 6/23/16 15:14, David Holmes wrote:
> On 23/06/2016 11:02 PM, stanislav lukyanov wrote:
>> There were different points in the discussion I didn't have a chance to
>> comment in time,
>> so I'll just summarize everything here not to break the thread flow with
>> answers to older messages.

Thank you for taking these steps.
It is very helpful.


>>
>> First of all, I'm perfectly fine with either approach (adding name
>> validation or not)
>> as long as the behavior is clearly documented.

Good.

>>
>> I think it should be specified that unnamed module will be returned even
>> if there cannot ever be a package
>> with that name. It is not a complicated case to specify, but it brings
>> much more clarity.
>> Formally, now the function is specified to work with "package names" and
>> the behavior on a string that is clearly not a "package name" is
>> unspecified.
>
> I think it is completely specified. Something that can not be a valid 
> package name can obviously not have a module associated with it and so 
> the unnamed-module is always returned.

I tend to agree with David here.
It is good that you guys having this discussion here as it is a way to 
reach a consensus.

>
>> The empty string case may or may not be specified. It may be considered
>> another corner case, different from both
>> "legal" and "illegal" package names, but I personally think that the
>> behavior is clear anyway.

Ok.

>>
>> I don't think the argument that JVMTI doesn't validate names is correct.
>> GetLocalVariableTable doesn't look like a good example here, since
>> it just reads data that's already in the VM anyway,
>> but in case of GetModuleByPackageName it is about validation of input
>> parameters.

Agreed here.


>> Other APIs that take identifiers like, for example, JNI FindClass or
>> GetMethodID
>> don't specify name checks explicitly, but throw
>> ClassNotFoundError/NoSuchMethodError illegal names.

The JNI does not do any check for illegal names.
If the name is illegal then the target is not found.
It is why the ClassNotFoundError/NoSuchMethodError is thrown.
It perfectly matches the current GetModuleByPackageName specification 
approach.


>> It looks like GetModuleByPackageName is the first JNI/JVMTI function to
>> succeed when an ill-formed identifier is passed,
>> so it deserves to be documented.
>
> I disagree with all of that. GetLocalVariableTable, ClassFileLoadHook, 
> DynamicCodeGenerated, GetThreadInfo, to name a few, all take "names" 
> encoded as UTF-8 modified strings. None of them validate that the 
> "name" is legal for the entity being named - JVM TI simply does not do 
> that kind of argument validation.

Right.

>
> The JNI functions also do not do argument validation. The JNI spec is 
> clear "The JNI does not check for programming errors such as passing 
> in NULL pointers or illegal argument types". It is up to the 
> programmer to ensure they pass valid arguments. FindClass is specified 
> to simply throw:
>
> NoClassDefFoundError: if no definition for a requested class or 
> interface can be found.
>
> It is the internal VM code, that has to deal with bytecode from 
> arbitrary sources, that performs the more detailed checking of the name.

Right.

>
> GetModuleByPackageName is slightly unusual in that it really never 
> fails. As I discussed in the CCC review it could have made a 
> distinction between packages that would be loaded by the loader and 
> packages that would not, and throw an exception (which in turn may 
> have been able to discern that the name was invalid). But that is not 
> the case - if you don't pass the name of a package that is known to 
> the loader then you get back the unnamed-module. It doesn't matter 
> whether the package name is legal-but-unknown, or illegal - it is just 
> unknown.

I tend to agree with David here.
But interested to know what objections to this can be.


Thanks,
Serguei

>
> David
> ------
>
>> On CCC update: AFAIU CCC needs to have final version of the proposed
>> specification, so yes,
>> it needs to be updated to with the test that will be actually pushed to
>> the workspace.
>>
>> Thanks,
>> Stas
>>
>> On 23.06.2016 14:40, David Holmes wrote:
>>>
>>>
>>> On 23/06/2016 9:33 PM, serguei.spitsyn at oracle.com wrote:
>>>> On 6/23/16 04:27, David Holmes wrote:
>>>>> On 23/06/2016 9:08 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> On 6/23/16 03:51, David Holmes wrote:
>>>>>>> On 23/06/2016 6:04 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> On 6/23/16 00:51, Alan Bateman wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 23/06/2016 00:20, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> :
>>>>>>>>>>
>>>>>>>>>> I agree with it.
>>>>>>>>>> Thank you for pointing to this JVMTI example.
>>>>>>>>>> I did not find in the JNI where the names are checked to be 
>>>>>>>>>> legal.
>>>>>>>>>> We are going to open a can of worms with this kind of check as
>>>>>>>>>> there
>>>>>>>>>> can be many corner cases to cover.
>>>>>>>>> The primary use-case for this function is from within the CLFH
>>>>>>>>> callback so have it return the unnamed module when calling with
>>>>>>>>> garage
>>>>>>>>> is probably okay, it just needs to be specified.
>>>>>>>>>
>>>>>>>>> Will you update the webrev to reflect where we've got to this in
>>>>>>>>> this
>>>>>>>>> discussion?
>>>>>>>>
>>>>>>>> I need to undo my fix for additional check.
>>>>>>>> My up-to-date understanding is that we have to explicitly specify
>>>>>>>> two
>>>>>>>> points:
>>>>>>>>   - the function returns the unnamed module if the package does 
>>>>>>>> not
>>>>>>>> exist
>>>>>>>>   - the function does not check the package names for validness 
>>>>>>>> and
>>>>>>>> always returns the unnamed module for illegal package names
>>>>>>>>
>>>>>>>> It is better to specify these points explicitly to avoid possible
>>>>>>>> confusion.
>>>>>>>
>>>>>>> I don't agree - nothing else refers to invalid "names" in JVM TI. I
>>>>>>> don't see any need to call this out here. If the name supplied 
>>>>>>> is not
>>>>>>> a legal package name then it will not match - end of story.
>>>>>>
>>>>>> David,
>>>>>>
>>>>>> It was the original approach that is in the CCC.
>>>>>> You were the one who got confused here, and it convinced me that 
>>>>>> this
>>>>>> needs to be more clear.
>>>>>> All these changes are because you started the discussion. :)
>>>>>> It was useful anyway.
>>>>>
>>>>> I never said anything about illegal package names.
>>>>
>>>> True.
>>>> This is my (probably, wrong) attempt to make it more clear.
>>>>
>>>>>
>>>>>> Are you against both clarifications or just the last one?
>>>>>> I'm Ok to return it back as it is in the CCC.
>>>>>> It will save time on the CCC approval.
>>>>>>
>>>>>> This is the last addition to the spec:
>>>>>>
>>>>>> + The unnamed module is returned if the specified package does not
>>>>>> exist.
>>>>>
>>>>> The notion of "package does not exist" is ill-defined. This case is
>>>>> already covered by the primary specification.
>>>>>
>>>>>> + The function does not check if the specified package name is
>>>>>> illegal.
>>>>>
>>>>> This does not need to be stated as it is not stated anywhere else for
>>>>> anything else that may have legal and illegal forms.
>>>>
>>>> Good.
>>>> This is a relevant fragment from current version of CCC:
>>>>
>>>> +      <description>
>>>> +        Return the <code>java.lang.reflect.Module</code> object for a
>>>> module
>>>> +        defined to a class loader that contains a given package.
>>>> +        The module is returned via <code>module_ptr</code>.
>>>> +        <p/>
>>>> +        If a named module is defined to the class loader and it
>>>> +        contains the package then that named module is returned,
>>>> +        otherwise the unnamed module of the class loader is returned.
>>>> +        If the package name is the empty string then this function
>>>> +        always returns the unnamed module for the class loader.
>>>> +        <p/>
>>>
>>> As Stanislav said explicitly mentioning the empty string is not really
>>> necessary - but I don't see it as harmful.
>>>
>>>> Does it looks Ok?
>>>
>>> Yes - as good now as it was in the CCC discussion :)
>>>
>>> Cheers,
>>> David
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> -Alan
>>>>>>>>
>>>>>>
>>>>
>>