Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Jun 29 02:29:31 UTC 2016
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.
>
> ---
>
> 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 hotspot-dev
mailing list