JVM/TI code review request (XS and M) (7182152)
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 "$PASS1_MESG" output.log | grep 'version-0' | wc -l`
> ---
> > cnt=`grep "$PASS1_MESG" output.log | grep 'version-0' | wc -l | sed
> 's/^ *//'`
> 145a146
> > ;;
> 149c150
> < cnt=`grep "$PASS2_MESG" output.log | grep 'version-1' | wc -l`
> ---
> > cnt=`grep "$PASS2_MESG" 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
>> - 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 serviceability-dev
mailing list