JVM/TI code review request (XS and M) (7182152)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 11 17:22:57 PST 2013
Serguei,
Thanks for the fast review!
I fixed the broken JBS link, but I can't do anything about getting
the bugs.sun.com entry out any faster. Sigh...
Dan
On 2/11/13 6:14 PM, serguei.spitsyn at oracle.com wrote:
> 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 hotspot-runtime-dev
mailing list