RFR 8005056: NPG: Crash after redefining java.lang.Object

[Coleen Phillimore]

Thank you Dan for the as usual thorough review.

On 5/10/2013 12:49 PM, Daniel D. Daugherty wrote:
> > open webrev at http://cr.openjdk.java.net/~coleenp/8005056/
>
> Thumbs up. Minor nits/questions below.
>
> Very nicely done. Adding a test to retransform java.lang.Object
> will do a nice job of detecting potential breakages in this area.
>

Yes, there was an older bug for redefine java/lang/Object in the system 
but I don't think it's the same.

>
> src/share/vm/classfile/dictionary.cpp
>     No comments.
>
> src/share/vm/classfile/dictionary.hpp
>     No comments.
>
> src/share/vm/classfile/systemDictionary.cpp
>     No comments.
>
> src/share/vm/classfile/systemDictionary.hpp
>     No comments.
>
> src/share/vm/oops/klass.hpp
>     No comments.
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
>     line 3547:  // print error if old methods are found.
>         We fail a guarantee here also so this is more than "print error".
>

I reworded:    // print and fail guarantee if old methods are found.
I would have capitalized Print but the other comments in this function 
didn't start with capital letters.

> line 3551    } else {

fixed.

> Indent is off by one space (not your change, but...)
>
> src/share/vm/prims/jvmtiRedefineClasses.hpp
>     No comments.
>
> test/testlibrary/ClassFileInstaller.java
>     line 43: String pathName = arg.replace('.', '/').concat(".class");
>     line 48: if (pathName.contains("/")) {
>         Should these be System.getProperty("path.separator")? Or maybe
>         this is something already queried and cached in this testing
>         harness?
>

I think this path refers to the Java path not the OS path name. 
Christian, can you confirm?


> test/runtime/RedefineObject/Agent.java
>     line 56: for (int i = 0; i< 1000 ; i++) {
>         Space before '<'.
>
fixed.

> test/runtime/RedefineObject/MANIFEST.MF
>      No comments (agree with Christian that this can be deleted)
>
fixed.

> test/runtime/RedefineObject/Test.java
>      No comments (agree with Christian that this should be renamed).
>

renamed to TestRedefineObject.java and added @bug and the comments that 
Christian requested.

thanks!
Coleen

> Dan
>
>
>
> On 5/9/13 8:22 AM, Coleen Phillimore wrote:
>>
>> Dan,
>>
>> Thank you for adding serviceability.  I'll provide more detail on the 
>> change if needed.  Here's some additional detail.
>>
>> What I did was added a closure to pass to 
>> ClassLoaderDataGraph::classes_do() function.  This function walks all 
>> the loaded classes, which includes the array classes.   The 
>> SystemDicitonary walk only walks loaded InstanceKlasses.   The array 
>> classes created are linked from the InstanceKlass in _array_klasses 
>> and the code used to walk them separately.   The code for switching 
>> to the new methods from the old methods didn't change very much.
>>
>> The reason we have to change the vtables in array classes is because 
>> array classes are inherited from java/lang/Object class and have this 
>> vtable.   We had missed the ones for arrays of basic types created in 
>> universe.  The old code never fixed these entries but it didn't crash 
>> because the methodOops were followed with the basic type array 
>> classes in their vtable so wouldn't go away. With permgen removal, we 
>> explicitly delete unreferenced Method objects so these ones have to 
>> be replaced.  And it's more correct because you don't want to call 
>> the old Method.
>>
>> Coleen
>>
>> On 05/09/2013 09:55 AM, Daniel D. Daugherty wrote:
>>> Adding Serviceability to this review thread since this concerns
>>> JVM/TI RedefineClasses().
>>>
>>> Coleen, this will take a bit of time to review.
>>>
>>> Dan
>>>
>>>
>>> On 5/8/13 8:51 PM, Coleen Phillimore wrote:
>>>> Summary: Need to walk array class vtables replacing old methods too 
>>>> if j.l.o is redefined
>>>>
>>>> Array methods aren't in the SystemDictionary and the code that was 
>>>> there didn't walk the basic type array classes defined in 
>>>> universe.   It also walked the same classes more than once. Use the 
>>>> ClassLoaderDataGraph class walking instead.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8005056/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8005056
>>>>
>>>> Tested with all redefine classes tests, jdk java/lang/instrument 
>>>> tests, hotspot jtreg tests.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>
>>
>