Dmitry, Sergeui, thank you for the review!

For the record, webrev.02: http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.02/ <http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.02/>


Dmitry

On 20.09.2016 13:39, Dmitry Samersoff wrote:
Dmitry,

Looks good for me!

Minor formatting nits:

libMAAClassFileLoadHook.c

103: missed space after ,

libMAAClassFileLoadHook.c:
266: extra semicolon

-Dmitry

On 2016-09-20 13:17, Dmitry Dmitriev wrote:
Serguei, Dmitry,

Thank you for the feedback! Here is an updated webrev.01;
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.01/
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.01/>

Dmitry

On 20.09.2016 12:48, Dmitry Samersoff wrote:
Dmitry,

Please, also change

!strcmp(...) to strcmp(...) == 0

because semantically strcmp result is not a boolean false.

-Dmitry

On 2016-09-20 06:38, serguei.spit...@oracle.com wrote:
Hi Dmitry,


Thanks a lot for this additional test coverage and discovering new bug:
    https://bugs.openjdk.java.net/browse/JDK-8165681

The tests look pretty good to me.

A couple of minor comments.

Dots are missed in the .c files comments.


http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/test/serviceability/jvmti




   265         printf("Expecting to find '%s' class in ClassLoad events
duringVM start phase.\n", EXPECTED_SIGNATURE);
   266         if (class_in_class_load_events_vm_start == JNI_FALSE) {
   267             throw_exc(env, "Unable to find expected class in
ClassLoad events duringstart phase!\n");
   268             return FAILED;
   269         }
   270
   271         if (class_prepare_events_vm_start_count == 0) {
   272             throw_exc(env, "Didn't get ClassPrepare events in
start
phase!\n");
   273             return FAILED;
   274         }
   275
   276         printf("Expecting to find '%s' class in ClassPrepare
events
duringVM start phase.\n", EXPECTED_SIGNATURE);
   277         if (class_in_class_prepare_events_vm_start == JNI_FALSE) {
   278             throw_exc(env, "Unable to find expected class in
ClassPrepare events duringstart phase!\n");
   279             return FAILED;
   280         }


     Could you, please, replace "start phase" with "early start phase"?
It will be more clear that the start phase mode is "early".

   288         if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
   289             throw_exc(env, "Class is found in ClassLoad events
duringVM Start phase!\n");
   290             return FAILED;
   291         }
   292
   293         printf("Expecting that '%s' class is absent in
ClassPrepare
events.\n", EXPECTED_SIGNATURE);
   294         if (class_in_class_prepare_events_vm_start == JNI_TRUE) {
   295             throw_exc(env, "Class is found in ClassPrepare events
duringVM Start phase!\n");
   296             return FAILED;
   297         }

     Could you, please, replace "VM Start phase" with "normal VM start
phase"?    It will be more clear that the start phase mode is "normal".
Thanks, Serguei On 9/19/16 05:47, Dmitry Dmitriev wrote:
Hello, Please review new tests for module aware agents. There are 3
tests: 1) MAAClassFileLoadHook.java - verifies ClassFileLoadHook event
with different combinations of can_generate_early_vmstart and
can_generate_early_class_hook_events JVMTI capabilities. Expects to
find(or not) class from java.base module in the right VM phase. 2)
MAAClassLoadPrepare.java - verifies ClassLoad and ClassPrepare events
with and without can_generate_early_vmstart JVMTI capability. Expects
to find(or not) class from java.base module in the VM start phase. 3)
MAAThreadStart.java - verifies ThreadStart event with
can_generate_early_vmstart JVMTI capability. Expect to find events in
the VM start phase. JBS:
https://bugs.openjdk.java.net/browse/JDK-8150758 webrev.00:
http://cr.openjdk.java.net/~ddmitriev/8150758/webrev.00/
<http://cr.openjdk.java.net/%7Eddmitriev/8150758/webrev.00/> Testing:
RBT on all platforms Thanks, Dmitry


Reply via email to