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

Tue Jun 28 18:19:51 UTC 2016

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

         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.

         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.

     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.

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.

     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'

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.

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

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.

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

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

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