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 >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
