[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-06-24 13:43:12
Message-ID: 576D38F0.5060403 () oracle ! com
[Download RAW message or body]

Stanislav and David,

Thank you for this interesting discussion!
It seems, there are good points from both sides.

Alan suggested to replace the GetModuleByPackageName with the 
GetNamedModule.
The GetNamedModule should return NULL in all cases where the unnamed 
module was returned by the GetModuleByPackageName.
It looks like this approach simplified the spec and will allow us to 
reach a consensus.

You can find new spec in the updated CCC.
I will work on the round #3 webrev and then send it for review.

(Sorry that I mentioned CCC in the open but it is very inconvenient to 
separate it in this email thread)

Please, let me know about your concerns.

Thanks,
Serguei


On 6/24/16 05:53, David Holmes wrote:
> On 24/06/2016 10:10 PM, stanislav lukyanov wrote:
>>
>> On 24.06.2016 2:07, serguei.spitsyn@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@oracle.com wrote:
>>>>>>> On 6/23/16 04:27, David Holmes wrote:
>>>>>>>> On 23/06/2016 9:08 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>> On 6/23/16 03:51, David Holmes wrote:
>>>>>>>>>> On 23/06/2016 6:04 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>>> On 6/23/16 00:51, Alan Bateman wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 23/06/2016 00:20, serguei.spitsyn@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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]