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

Tue Jun 28 20:11:42 UTC 2016

On 6/28/16 11:19, Daniel D. Daugherty wrote:
> On 6/28/16 10:37 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Harold,
>>
>> Thank you again for the comments!
>> This is the updated webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.5/ 
>>
>
> make/test/JtregNative.gmk
>     No comments.
>
> src/share/vm/classfile/modules.hpp
>     No comments.
>
> src/share/vm/classfile/modules.cpp
>     L826:   assert(h_loader.is_null() || 
> java_lang_ClassLoader::is_subclass(h_loader->klass()),
>     L827:         "Class loader is not a subclass of 
> java.lang.ClassLoader");
>         nit: indent on L827 is off by 1 space

Thanks, will fix.

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

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

>
>     L828:   assert(package_str != NULL, "the package_str should not be 
> NULL");
>         Same concern about the package_str parameter.
>
>         Update: Yes, the generated JVM/TI glue code should catch a
>             NULL package_name/package_str at the upper layers.

Yes, but this assert is intentional as above.

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

>
>     L2518:     function can be used to map class loader and package 
> name to a module.
>         Typo: 'map class loader and package name'
>            -> 'map a class loader and a package name'

Good catch, thanks!
Will fix.

>
>
> src/share/vm/prims/jvmtiEnv.cpp
>     L204: // class_loader - NULL is a valid value, must be pre-checked
>     L205: // package_name - pre-checked for NULL
>     L206: // module_ptr - pre-checked for NULL
>         The jvmti.xml file doesn't mark package_name or module_ptr
>         with <nullok> so both of those should be checked by the
>         generated glue code.
>
>         class_loader is marked with <nullok> so a NULL class_loader
>         will get to here:
>
>         L217:   jobject module = Modules::get_named_module(h_loader, 
> package_name, THREAD);
>
>         and it looks like all the code paths in get_named_module()
>         properly account for both NULL and non-NULL h_loader. Cool.

Good.
Thank you for checking.

>
> test/serviceability/jvmti/GetNamedModule/MyPackage/GetNamedModuleTest.java 
>
>     L42:             System.err.println("java.library.path:"
>         nit: a space between the ':' and '"' would make the
>              output more readable

Thanks, will fix.

>
> test/serviceability/jvmti/GetNamedModule/libGetNamedModuleTest.c
>     L81:     res = JNI_ENV_PTR(jvm)->GetEnv(JNI_ENV_ARG(jvm, (void **) 
> &jvmti),
>     L82:         JVMTI_VERSION_1_1);
>         I was expecting this test to ask for the JVM/TI version that
>         includes support for GetAllModules() and GetNamesModule().
>         Looks like that is version 9.0.0 or newer aka JVMTI_VERSION_9.

You are right.
Will fix.

>
>     L360:     if (module != NULL || mod_name !=NULL) {
>         bit: space after '!=' and before NULL

Thanks, will fix.

>
> Has someone checked the generated glue code for completeness and
> proper checks?

I checked it some time ago.
But will double-check it.

Thank you for the review and detailed comments!
Serguei

>
> Dan
>
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 6/28/16 07:32, harold seigel wrote:
>>>
>>> Hi Serguei,
>>>
>>> Looks good.
>>>
>>> 1. In modules.cpp, the first assert in get_named_module() has the 
>>> wrong function name:
>>>
>>> 825 assert(ModuleEntryTable::javabase_defined(),
>>> 826 "Attempt to call *get_module_by_package_name* before java.base 
>>> is defined");
>>>
>>> 2. Also, is a check needed to ensure that package_str is not null?
>>>
>>> 3. Is the ResourceMark(THREAD) needed at line 824?
>>>
>>> Thanks, Harold
>>>
>>> On 6/28/2016 4:54 AM, serguei.spitsyn at oracle.com wrote:
>>>> Hi David,
>>>>
>>>> Thank you for the comments!
>>>> I agree with you.
>>>>
>>>> Updated Hotspot webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.4/ 
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>
>>>> On 6/27/16 22:06, David Holmes wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Overall this looks a lot clearer/simpler.
>>>>>
>>>>> On 28/06/2016 2:20 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> On 6/27/16 21:08, serguei.spitsyn at oracle.com wrote:
>>>>>>> Please, review the Jigsaw fix for the enhancement:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8159145
>>>>>>>
>>>>>>>
>>>>>>> The Hotspot webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.3/ 
>>>>>>>
>>>>>
>>>>> Are there intended to be other callers of 
>>>>> Module::get_named_module? If not it seems odd to go to all the 
>>>>> trouble of throwing exceptions, just to have the caller clear them 
>>>>> and return an error code. Or you could move all that argument 
>>>>> checking code into the JVMTI function directly - if called 
>>>>> internally you would just assert that arguments are as expected.
>>>>>
>>>>>
>>>>> src/share/vm/prims/jvmti.xml
>>>>>
>>>>> +         otherwise the <code>NULL</code> is returned.
>>>>>
>>>>> Delete "the".
>>>>>
>>>>> Otherwise all looks good to me.
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The Jdk webrev is the same:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/ 
>>>>>>>
>>>>>>
>>>>>> Sorry, the Jdk webrev changed as well:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk3/ 
>>>>>>
>>>>>
>>>>> Looks fine.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>>   This is the review round #3.
>>>>>>>   Alan suggested to replace the function GetModuleByPackageName 
>>>>>>> with
>>>>>>> the GetNamedModule.
>>>>>>>   New function will return NULL if the package is not in a module
>>>>>>> defined to the class loader.
>>>>>>>   It simplifies the API and makes it easier to specify.
>>>>>>>   JVM TI agents that instrument code in named modules need the 
>>>>>>> Module
>>>>>>> at class load time.
>>>>>>>
>>>>>>>   One question that came from the function semantics change.
>>>>>>>   I had to implement the Modules::get_named_module() from scratch
>>>>>>> independently of the existing
>>>>>>>   Modules::get_module_by_package_name() and Modules::get_module().
>>>>>>>   The issue is that the Modules::get_module() can return the 
>>>>>>> unnamed
>>>>>>> module whereas the
>>>>>>>   JVMTI helper Modules::get_named_module() should return NULL 
>>>>>>> instead
>>>>>>> of the unnamed module.
>>>>>>>   Please, let me know if it is Ok or if you have better ideas 
>>>>>>> how to
>>>>>>> share the code.
>>>>>>>
>>>>>>>   This is the Summary from review round #1:
>>>>>>>
>>>>>>>   One way to do this is by introducing a new ClassFileLoadHook that
>>>>>>> takes an additional parameter but this approach is disruptive.
>>>>>>>   The alternative option is a JVM TI function that maps a 
>>>>>>> classloader
>>>>>>> + package name to a module.
>>>>>>>   We were initially not confident with this approach so we 
>>>>>>> introduced
>>>>>>> it as JVM function JVM_GetModuleByPackageName.
>>>>>>>   Based on experience to date then this approach seems okay and so
>>>>>>> this function needs to be promoted to a JVMTI function.
>>>>>>>
>>>>>>>   It includes new jtreg test with native JVMTI agent.
>>>>>>>
>>>>>>>
>>>>>>> Testing:
>>>>>>>    Run newly developed jtreg test.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>
>>>>
>>>
>>
>