JVM/TI code review request (XS and M) (7182152)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Feb 7 00:30:06 PST 2013
I agree, the test is a piece of art - nice example to start from for
future test development.
Ship it!
Thanks,
Serguei
On 2/6/13 12: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
> - RedefineSubclassWithTwoInterfacesImpl_1.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: <begin output.log>"
> 102a106,107
> > echo "INFO: <end output.log>"
> >
> 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".
> > #
> >
> > FAIL_MESG="guarantee"
> > grep "$FAIL_MESG" output.log
> > status=$?
> > if [ "$status" = 0 ]; then
> > echo "FAIL: found '$FAIL_MESG' in the test output."
> 115c130,131
> < echo "PASS: did NOT find '$MESG' in the test output"
> ---
> > echo "INFO: did NOT find '$FAIL_MESG' in the test output."
> > # be optimistic here
> 118a135,164
> > PASS1_MESG="before any redefines"
> > cnt=`grep "$PASS1_MESG" output.log | grep 'version-0' | wc -l`
> > case "$cnt" in
> > 2)
> > echo "INFO: found 2 version-0 '$PASS1_MESG' mesgs."
> > ;;
> > *)
> > echo "FAIL: did NOT find 2 version-0 '$PASS1_MESG' mesgs."
> > echo "INFO: grep '$PASS1_MESG' output:"
> > grep "$PASS1_MESG" output.log
> > result=1
> > esac
> >
> > PASS2_MESG="after redefine"
> > cnt=`grep "$PASS2_MESG" output.log | grep 'version-1' | wc -l`
> > case "$cnt" in
> > 2)
> > echo "INFO: found 2 version-1 '$PASS2_MESG' mesgs."
> > ;;
> > *)
> > echo "FAIL: did NOT find 2 version-1 '$PASS2_MESG' mesgs."
> > echo "INFO: grep '$PASS2_MESG' output:"
> > grep "$PASS2_MESG" 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
> > // RedefineSubclassWithTwoInterfacesImpl_1.java are identical.
>
>
> $ diff
> test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl_1.java{.cr0,}
> 23a24,27
> > // Reproducing this bug only requires an EMCP version of the
> > // RedefineSubclassWithTwoInterfacesImpl class so
> > // RedefineSubclassWithTwoInterfacesImpl.java and
> > // RedefineSubclassWithTwoInterfacesImpl_1.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/view_bug.do?bug_id=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/view_bug.do?bug_id=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 @@
>>>> // RC_TRACE macro has an embedded ResourceMark
>>>> RC_TRACE(0x00200000, ("itable method update: %s(%s)",
>>>> new_method->name()->as_C_string(),
>>>> new_method->signature()->as_C_string()));
>>>> }
>>>> - 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-jdi_closed
>>>> 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 hotspot-runtime-dev
mailing list