Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Jun 28 20:42:24 UTC 2016
Dan,
The updated webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.6/
> 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.
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