Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule
David Holmes
david.holmes at oracle.com
Wed Jun 29 02:37:26 UTC 2016
On 29/06/2016 12:29 PM, serguei.spitsyn at oracle.com wrote:
> Hi David,
>
>
> On 6/28/16 19:17, David Holmes wrote:
>> Hi Serguei,
>>
>> Looks good to me. A couple of comments ...
>
> Thank you for reviewing it!
>
>
>>
>> On 29/06/2016 6:42 AM, serguei.spitsyn at oracle.com wrote:
>>> Dan,
>>>
>>> The updated webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.6/
>>>
>>
>> src/share/vm/classfile/modules.cpp
>>
>> Nit:
>>
>> + const PackageEntry* const pkg_entry =
>> + get_package_entry_by_name(package_sym, h_loader, THREAD);
>>
>> indent on a line continuation should be 4 not 2. (no need to see
>> updated webrev).
>
> This fragment was taken from the function get_module() with the
> pre-existing indent.
> There are a couple of other fragments with the indent like this.
> There is a rule to follow the the file style.
<sigh> There are a number of indentation and line-length issues in this
new file.
David
>
>>
>> ---
>>
>> src/share/vm/prims/jvmti.xml
>>
>> + or points to <code>NULL</code>.
>>
>> I would have suggested "or is <code>NULL</code>." as per other email,
>> but I see that this terminology is pre-existing, so ignore that other
>> email.
>
> Ok, thanks!
>
>
>>
>>>
>>>
>>>> Has someone checked the generated glue code for completeness and
>>> proper checks?
>>>
>>> This is the generated upper layer function in the jvmtiEnter.cpp:
>>>
>>> 3185 static jvmtiError JNICALL
>>> 3186 jvmti_GetNamedModule(jvmtiEnv* env,
>>> 3187 jobject class_loader,
>>> 3188 const char* package_name,
>>> 3189 jobject* module_ptr) {
>>> 3190
>>> 3191 #if !INCLUDE_JVMTI
>>> 3192 return JVMTI_ERROR_NOT_AVAILABLE;
>>> 3193 #else
>>> 3194 if(!JvmtiEnv::is_vm_live()) {
>>> 3195 return JVMTI_ERROR_WRONG_PHASE;
>>> 3196 }
>>> 3197 Thread* this_thread = Thread::current_or_null();
>>> 3198 if (this_thread == NULL || !this_thread->is_Java_thread()) {
>>> 3199 return JVMTI_ERROR_UNATTACHED_THREAD;
>>> 3200 }
>>> 3201 JavaThread* current_thread = (JavaThread*)this_thread;
>>> 3202 ThreadInVMfromNative __tiv(current_thread);
>>> 3203 VM_ENTRY_BASE(jvmtiError, jvmti_GetNamedModule , current_thread)
>>> 3204 debug_only(VMNativeEntryWrapper __vew;)
>>> 3205 CautiouslyPreserveExceptionMark __em(this_thread);
>>> 3206 JvmtiEnv* jvmti_env = JvmtiEnv::JvmtiEnv_from_jvmti_env(env);
>>> 3207 if (!jvmti_env->is_valid()) {
>>> 3208 return JVMTI_ERROR_INVALID_ENVIRONMENT;
>>> 3209 }
>>> 3210 jvmtiError err;
>>> 3211 if (package_name == NULL) {
>>> 3212 return JVMTI_ERROR_NULL_POINTER;
>>> 3213 }
>>> 3214 if (module_ptr == NULL) {
>>> 3215 return JVMTI_ERROR_NULL_POINTER;
>>> 3216 }
>>> 3217 err = jvmti_env->GetNamedModule(class_loader, package_name,
>>> module_ptr);
>>> 3218 return err;
>>> 3219 #endif // INCLUDE_JVMTI
>>> 3220 }
>>>
>>>
>>> I do not see any problems in the above.
>>
>> Looks good to me too.
>
> Good.
>
>
> Thanks!
> Serguei
>
>
>>
>> Thanks,
>> David
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>>
>>> 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
>>>>
>>>> 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
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
More information about the serviceability-dev
mailing list