JVM/TI code review request (XS and M) (7182152)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Feb 11 17:14:40 PST 2013
This looks good.
BTW, the bug report links in the webrev are not resolved.
It was not a big problem for me, just wanted to let you know.
Thanks,
Serguei
On 2/11/13 4:41 PM, Daniel D. Daugherty wrote:
> Greetings,
>
> Right after I pushed the changeset for 8007420, Alan B let me know that
> he had updated the JLI/JPLIS test infrastructure to support running the
> tests with a JRE instead of a JDK. Alan made his changes in early
> January,
> but I had developed this test back in December. I never checked to see
> if the test on which I based my new test had changed. Sigh...
>
> I filed a new bug to update the new test. Here is the webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8007935-webrev/0-jdk8-tl/
>
> As always, comments and suggestions are welcome.
>
> Dan
>
>
>
> On 2/11/13 8: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