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

Coleen Phillimore coleen.phillimore at oracle.com
Mon Feb 4 13:48:20 PST 2013 

Ok, my poor eyes.   One short comment below.

On 2/4/2013 4:22 PM, Daniel D. Daugherty wrote:
> Thanks for the additional test comments. Replies embedded below.
>
>
> On 2/4/13 1:06 PM, Coleen Phillimore wrote:
>> On 2/4/2013 12:19 PM, Daniel D. Daugherty wrote:
>>> Adding back dropped aliases and people...
>>
>> I think I have all the aliases on this, this time.
>
> And the individual people also. Thanks!
>
>
>>>
>>> Thanks for reviewing the first test! Replies embedded below.
>>>
>>
>> Ok, now I see about the scaffolding framework for the first test. I 
>> can say it looks consistent with the other tests in that directory.
>>
>> I reviewed the second test too.  I have some questions.
>>
>> in 
>> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh.html
>>
>> In lines 70-77 you move the same two files twice.
>
> I don't think so. Getting rid of the
> "RedefineSubclassWithTwoInterfaces" prefix will make the
> real script code more apparent:
>
>   70 mv X_Target.java \
>   71     X_Target_1.java
>   72 mv X_Target.class \
>   73     X_Target_1.class
>   74 mv X_Impl.java \
>   75     X_Impl_1.java
>   76 mv X_Impl.class \
>   77     X_Impl_1.class
>
>> Also the file,
>>
>> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl_1.java.html 
>>
>>
>> Is exactly the same as the file 
>> RedefineSubclassWithTwoInterfacesImpl.java
>>
>> Isn't one supposed to be derived from Target_1 rather than Target or 
>> different in some way?
>
> The "_1" stuffis just for version naming purposes. The actual
> class name has to be the same between Foo.java and Foo_1.java.
>
> In this particular bug's case, we only needed to redefine
> RedefineSubclassWithTwoInterfacesImpl with an EMCP version
> to tickle the bug. That's why there are no differences
> between RedefineSubclassWithTwoInterfacesImpl.java and
> RedefineSubclassWithTwoInterfacesImpl_1.java.
>
> Good eyes though.

Can you put a comment about why there are no differences but it's a 
different file?

>
>
>> This second test makes more sense to me than the first anyway. Are 
>> you going to wait for the fix to get propagated before checking it in?
>
> Yes, I have to wait until the HSX fix has made it to the T&L repo
> before I can push the tests. Integrating failing tests intentionally
> is not permitted in T&L. Of course, you can integrate them disabled,
> but then you have to go back and enable the test sometime.

Yes, this is somewhat of a drag.

Everything else looks fine.
Thanks!
Coleen

>
> Dan
>
>
>> Thanks,
>> Coleen
>>
>>
>>>
>>> On 2/4/13 8:09 AM, Coleen Phillimore wrote:
>>>> On 2/1/2013 6: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/
>>>>
>>>> I am so impressed that you were able to write a test for this.
>>>
>>> Thanks. The shame of it is that I wrote this test back when I fixed
>>> the 1-liner bug back in 2009. The test never got pushed to the JDK
>>> repo so the regression was able to sneak in later. Sigh.
>>>
>>>
>>>> I can't follow this shell script at all. Who calls createJavaFile?
>>>
>>> This test is written using the JDI ShellScaffold.sh infrastructure
>>> in test/com/sun/jdi/ShellScaffold.sh (1106 lines of pure joy!).
>>> createJavaFile() is a hook called by ShellScaffold.sh to create
>>> the .java file that will be exercised by the test.
>>>
>>>
>>>> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/com/sun/jdi/RedefineAbstractClass.sh.html 
>>>>
>>>>
>>>> I think a limited amount of duplication of Java code in the test 
>>>> directory is worth having these scripts be more simple. It looks 
>>>> like the only thing that substituted in the java test is the main 
>>>> class, but lines 66 to 104 can be a stand-alone java source file.
>>>
>>> No, the main class just has the breakpoints.
>>>
>>> This is the line for the redefinition:
>>>
>>>   76     // @1 uncomment System.out.println("This is call " + 
>>> counter + " to checkFunc");
>>>
>>> so the original version doesn't have this line and new version
>>> has this line. The glue that makes all that work is in TestScaffold.sh.
>>>
>>> I don't remember for sure, but I think JDI ShellScaffold.sh does not
>>> work well with multiple .java files. It has been a long time since
>>> I've written a test in this format. These JDI .sh tests are meant to
>>> be standalone if need be. That's why this comment is always here:
>>>
>>>  143 # You could replace this next line with the contents
>>>  144 # of ShellScaffold.sh and this script will run just the same.
>>>  145 mysetup
>>>
>>>
>>>> Again, I can't really follow the logic here and I dread ever having 
>>>> to debug this.   Can you simplify this?
>>>
>>> Which part? By definition, RedefineClasses tests are complicated 
>>> because
>>> you have to have at least two versions of .class files, sometimes more.
>>> This test is reproducing a bug using "jdb" so there is all the infra
>>> that supports controlling "jdb".
>>>
>>> I think this test is as simple as it can be given the context, but I'm
>>> open to specific simplification suggestions.
>>>
>>> Did you plan to review the java.lang.instrument (JLI) test? It's a bit
>>> more convoluted because it is trying to mimic the WLS test case.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> 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