On 12/15/2010 9:37 PM, Keith McGuigan wrote:
Ok, here's a new webrev:
http://cr.openjdk.java.net/~kamg/6436034/webrev.01/
I added a regression test and modified the code in debugInit.cpp to
explicitly allow running with JVMTI 1.1 if that's what the JVM
supports. The regression test is setup to pass when run with a JVM
less than version 20 build 05 (where the new JVMTI function is added),
so it should run and pass in either situation and won't require a
later update.
General comments:
- To reiterate, I like the idea of being able to drop an older VM
into JDK7 in order to triage failures. This is particularly
important as we try to spin up the Serviceability Team again
and are working through the enormous bug backlog.
- Since the JDK7 project plan has been morphed into the JDK7 and
JDK8 plans, I don't think we have to worry about any more JVM/TI
API addition in the JDK7 timeframe.
- This JDK code will also be likely back ported into OpenJDK6 so
that release train will also have the same "drop in a VM"
capability.
- don't forget to update the copyright years in the various files
- block comment style is different than existing code. Should be:
/*
* ...
*/
src/share/back/debugInit.c
Yes, keep the JVM/TI version 0.x compatibility check. It doesn't
really cost much and you never know what crazy things we'll try
to do when triaging.
No content comments.
src/share/back/eventFilter.c
Don't really need the 'else' on line 292.
src/share/javavm/export/jvmti.h
Any idea why there is a new blank line at 2534? Will jcheck
complain about that?
test/com/sun/jdi/NativeInstanceFilter.java
Didn't review closely since I think you'll be tweaking the test.
test/com/sun/jdi/NativeInstanceFilterTarg.java
Didn't review closely since I think you'll be tweaking the test.
