On 29/06/2016 12:29 PM, [email protected] 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, [email protected] 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, [email protected] 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, [email protected] 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, [email protected] wrote:
On 6/27/16 21:08, [email protected] 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
