JVM/TI code review request (XS and M) (7182152) (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Mon Feb 11 07:34:55 PST 2013

Looks good, although I was surprised you don't need -e.

Coleen

On 02/11/2013 10:05 AM, Daniel D. Daugherty wrote:

Greetings,

I've revised one of the tests to make it more portable based on JPRT test results for the Code Review Round 1 version. Here is the URL for the webrev for Code Review Round 2: http://cr.openjdk.java.net/~dcubed/8007420-webrev/2-jdk8-tl/ Summary of the changes: - RedefineSubclassWithTwoInterfaces.sh - strip leading white space from 'wc -l' output for portable case statement check of "$cnt". - add missing ';;' for default cases (style only) Diffs are below since the webrev above is relative to JDK8-T&L and everything is "new". Dan $ diff test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh{.cr1,} 136c136 < cnt=grep "$PASS1MESG" output.log | grep 'version-0' | wc -l_ _---_ _> cnt=grep "$PASS1MESG" output.log | grep 'version-0' | wc -l | sed_ _'s/^ *//' 145a146 > ;; 149c150 < cnt=grep "$PASS2MESG" output.log | grep 'version-1' | wc -l_ _---_ _> cnt=grep "$PASS2MESG" output.log | grep 'version-1' | wc -l | sed_ _'s/^ *//' 158a160 > ;;

On 2/6/13 1:08 PM, Daniel D. Daugherty wrote: Greetings, I've revised the tests based on Coleen's and Serguei's feedback in Code Review Round 0. Here is the URL for the webrev for Code Review Round 1: http://cr.openjdk.java.net/~dcubed/8007420-webrev/1-jdk8-tl/ Summary of the changes: - RedefineAbstractClass.sh - cleaned up the header comment - RedefineAbstractClass.sh - more clear description of what's being tested - add comments describing what the guarantee failure looks like - add checks for proper pre-RedefineClasses output - add checks for proper post-RedefineClasses output - RedefineSubclassWithTwoInterfacesApp.java - RedefineSubclassWithTwoInterfacesRemote.java - refactor the test app's echo() method call into echo1() and echo2() so it is easier to check for the failure mode - RedefineSubclassWithTwoInterfacesImpl.java - RedefineSubclassWithTwoInterfacesImpl1.java - add comment to explain why these sources are identical

Diffs are below since the webrev above is relative to JDK8-T&L and everything is "new". Dan Here are diffs (ignoring whitespace) to make it clear what has changed between Code Review Round 0 and Code Review Round 1: $ diff -w test/com/sun/jdi/RedefineAbstractClass.sh{.cr0,} 31a32 > # @author Daniel D. Daugherty $ diff -w test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh{.cr0,} 26c26,27 < # @summary Redefining a subclass that implements two interfaces is_ _broken._ _---_ _> # @summary Redefine a subclass that implements two interfaces and > # verify that the right methods are called. 101a103,104 > > echo "INFO: " 102a106,107 > echo "INFO: " > 108,112c113,127 < MESG="guarantee"_ _< grep "$MESG" output.log_ _< result=$?_ _< if [ "$result" = 0 ]; then_ _< echo "FAIL: found '$MESG' in the test output"_ _---_ _> # When this bug manifests, RedefineClasses() will fail to update > # one of the itable entries to refer to the new method. The log > # will include the following line when the bug occurs: > # > # guarantee(false) failed: OLD and/or OBSOLETE method(s) found > # > # If this guarantee happens, the test should fail in the status > # check above, but just in case it doesn't, we check for "guarantee". > # > > FAILMESG="guarantee" > grep "$FAILMESG" output.log > status=$? > if [ "$status" = 0 ]; then > echo "FAIL: found '$FAILMESG' in the test output." 115c130,131 < echo "PASS: did NOT find '$MESG' in the test output"_ _---_ _> echo "INFO: did NOT find '$FAILMESG' in the test output." > # be optimistic here 118a135,164 > PASS1MESG="before any redefines" > cnt=grep "$PASS1MESG" output.log | grep 'version-0' | wc -l > case "$cnt" in > 2) > echo "INFO: found 2 version-0 '$PASS1MESG' mesgs." > ;; > *) > echo "FAIL: did NOT find 2 version-0 '$PASS1MESG' mesgs." > echo "INFO: grep '$PASS1MESG' output:" > grep "$PASS1MESG" output.log > result=1 > esac > > PASS2MESG="after redefine" > cnt=grep "$PASS2MESG" output.log | grep 'version-1' | wc -l > case "$cnt" in > 2) > echo "INFO: found 2 version-1 '$PASS2MESG' mesgs." > ;; > *) > echo "FAIL: did NOT find 2 version-1 '$PASS2MESG' mesgs." > echo "INFO: grep '$PASS2MESG' output:" > grep "$PASS2MESG" output.log > result=1 > esac > > if [ "$result" = 0 ]; then > echo "PASS: test passed both positive and negative output checks." > fi > $ diff test/java/lang/instrument/RedefineSubclassWithTwoInterfacesApp.java{.cr0,} 43,45c43,45 < // make an echo() call before any redefinitions:_ _< mesg = remote.echo("test message #0");_ _< System.out.println("RedefineSubclassWithTwoInterfacesApp: mesg='"_ _---_ _> // make echo() calls before any redefinitions: > mesg = remote.echo1("test message #1.1"); > System.out.println("RedefineSubclassWithTwoInterfacesApp: echo1 mesg='" 46a47,49 > mesg = remote.echo2("test message #2.1"); > System.out.println("RedefineSubclassWithTwoInterfacesApp: echo2 mesg='" > + mesg + "' before any redefines"); 54,56c57,62 < mesg = remote.echo("test message #1");_ _< System.out.println("RedefineSubclassWithTwoInterfacesApp: mesg='"_ _< + mesg + "' after redefine #1");_ _---_ _> mesg = remote.echo1("test message #1.2"); > System.out.println("RedefineSubclassWithTwoInterfacesApp: echo1 mesg='" > + mesg + "' after redefine"); > mesg = remote.echo2("test message #2.2"); > System.out.println("RedefineSubclassWithTwoInterfacesApp: echo2 mesg='" > + mesg + "' after redefine"); $ diff test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl.java{.cr0,} 23a24,27 > // Reproducing this bug only requires an EMCP version of the > // RedefineSubclassWithTwoInterfacesImpl class so > // RedefineSubclassWithTwoInterfacesImpl.java and > // RedefineSubclassWithTwoInterfacesImpl1.java are identical. $ diff test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl1.java{.cr0,} 23a24,27 > // Reproducing this bug only requires an EMCP version of the > // RedefineSubclassWithTwoInterfacesImpl class so > // RedefineSubclassWithTwoInterfacesImpl.java and > // RedefineSubclassWithTwoInterfacesImpl1.java are identical. $ diff test/java/lang/instrument/RedefineSubclassWithTwoInterfacesRemote.java{.cr0,} 40,41c40,41 < public String echo(String s) {_ _< return "intf1<" + intf1.echo(s) + "> intf2< " +_ _intf2.echo(s) + ">"; --- > public String echo1(String s) { > return "intf1<" + intf1.echo(s) + ">"; 42a43,46 > > public String echo2(String s) { > return "intf2<" + intf2.echo(s) + ">"; > } On 2/1/13 4:55 PM, Daniel D. Daugherty wrote: And here is the webrev for the new tests (relative to JDK8-T&L): http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/ As always, comments and suggestions are welcome. Dan

On 2/1/13 4:39 PM, Daniel D. Daugherty wrote: > There are two new tests that will be pushed to the JDK repos using > a different bug ID (not yet filed): New bug is now filed: 8007420 add test for 6805864 to com/sun/jdi, add test for 7182152 to java/lang/instrument http://bugs.sun.com/bugdatabase/viewbug.do?bugid=8007420 https://jbs.oracle.com/bugs/browse/JDK-8007420 Of course, the tests cannot be pushed until the HSX changes have made it into a promoted build and thus available to JDK8-T&L. Dan

On 2/1/13 12:55 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following JVM/TI bug: 7182152 Instrumentation hot swap test incorrect monitor count http://bugs.sun.com/bugdatabase/viewbug.do?bugid=7182152 https://jbs.oracle.com/bugs/browse/JDK-7182152 The fix for the bug in the product code is one line: src/share/vm/oops/klassVtable.cpp: @@ -992,18 +1020,50 @@ // RCTRACE macro has an embedded ResourceMark RCTRACE(0x00200000, ("itable method update: %s(%s)", newmethod->name()->asCstring(), newmethod->signature()->asCstring())); } - break; + // cannot 'break' here; see for-loop comment above. } ime++; } } } and is applicable to JDK7u10/HSX-23.6 and JDK7u14/HSX-24. Coleen already fixed the bug as part of the Perm Gen Removal (PGR) project in HSX-25. Yes, we found a 1-line bug fix buried in the monster PGR changeset. Many thanks to Coleen for her help in this bug hunt! The rest of the code in the webrevs are: - additional JVM/TI tracing code backported from Coleen's PGR changeset - additional JVM/TI tracing code added by me and forward ported to HSX-25 - a new -XX:TraceRedefineClasses=16384 flag value for finding these elusive old or obsolete methods - exposure of some printing code to the PRODUCT build so that the new tracing is available in a PRODUCT build You might be wondering why the new tracing code is exposed in a PRODUCT build. Well, it appears that more and more PRODUCT bits deployments are using JVM/TI RedefineClasses() and/or RetransformClasses() at run-time to instrument their systems. This bug (7182152) was only intermittently reproducible in the WLS environment in which it occurred so I made the tracing available in a PRODUCT build to assist in the hunt. Raj from the WLS team has also verified that the HSX-23.6 version of fix resolves the issue in his environment. Thanks Raj! Here are the URLs for the three webrevs: http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx23.6/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx24/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx25/ I have run the following test suites from the JPDA stack on the JDK7u10/HSX-23.6 version of the fix with -XX:TraceRedefineClasses=16384 specified: sdk-jdi sdk-jdiclosed sdk-jli vm-heapdump vm-hprof vm-jdb vm-jdi vm-jdwp vm-jvmti vm-sajdi The tested configs are: {Solaris-X86, WinXP} X {Client VM, Server VM} X {-Xmixed, -Xcomp} X {product, fastdebug} With the 1-liner fix in place, the new tracing code does not find any instances of this failure mode in any of the above test suites. Without the the 1-liner fix in place, the new tracing code finds one instance of this failure mode in the above test suites: test/java/lang/instrument/IsModifiableClassAgent.java There are two new tests that will be pushed to the JDK repos using a different bug ID (not yet filed): test/com/sun/jdi/RedefineAbstractClass.sh test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh There will be a separate review request for the new tests. I'm currently running the JPDA stack of tests on the JDK7u14/HSX-24 and JDK8-B75/HSX-25 versions of the fix. That testing will likely take all weekend to complete. Thanks, in advance, for any comments and/or suggestions. Dan

More information about the serviceability-dev mailing list