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

[David Holmes]

On 24/06/2016 10:10 PM, stanislav lukyanov wrote:
>
> On 24.06.2016 2:07, serguei.spitsyn at oracle.com wrote:
>> On 6/23/16 15:14, David Holmes wrote:
>>> On 23/06/2016 11:02 PM, stanislav lukyanov wrote:
>>>>
>>>> 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.
>>
>>>> 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.
> Yes, sure, there are no special checks.
> What I meant is that the functions do not succeed with illegal names,
> and because of that don't need such checks, but that's different with the
> GetModuleByPackageName since it does succeed.

It is a matter of perspective. This function simply says "if you give me 
a package known to this classloader I will give you its module; in all 
other cases I will give you the unnamed module".

>>>> 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.
> All these functions don't take the names as input from user.
> GetLocalVariableTable and GetThreadInfo return the names, not accept them.
> ClassFileLoadHook and DynamicCodeGenerated are called by JVM, not user.
> So I see these cases as completely different.

Sorry yes you are right these functions fill in structs with the name. I 
misread that.

So there are really no JVM TI functions that take in a name.

>>>
>>> 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.
>>
> As I've said above, FindClass sure don't specify (and, probably,
> perform) validation, but it is important that it will always fail with
> an illegal argument
> (even if it's with NoClassDefFoundError and not something like
> IllegalArgumentException)

Sure but that is just the way it operates. Look at it this way, 
FindClass could have been specified to operate as follows:
- returns the class of the given (valid) name; else
- throws NoClassDefFoundError for an unknown (but valid) class; else
- throws IllegalArgumentException if the name could not be a class name

but it isn't. An invalid name is a class not found - the fact the 
exception says "by the way that was not a valid class name" is just an 
incidental effect of the internal implementation.

>>>
>>> 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.
>>
> My concern is that the spec doesn't give straightforward answers
> to how the function may be called
> ("Can I pass an arbitrary string, will it just return unnamed module,
> will I break something?")
> and, more importantly, implemented
> ("Can I only care about valid names, can I implement it in a way that
> doesn't consider illegal characters?").
> I feel that could be clarified, but If that's just me still and you
> still feel that
> it is clear already then I think it's OK to go with the original version.
>
> BTW if the spec doesn't give special treatment to illegal names
> then the empty string clause should go away - if "otherwise..." clause
> is good enough
> to stand for "\t\n!@#$%^&*", it is definitely good enough to stand for "".

I agree the "" case is already covered.

David
-----

> Thanks,
> Stas
>>
>> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>