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

[Daniel D. Daugherty]

 > 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.


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".

     line 3551    } else {
         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?

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

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

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

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
>>>
>>
>