Request for review (S) 6512830: Error: assert(tag_at(which).is_unresolved_klass(), "Corrupted constant,pool")

[Coleen Phillimore]

On 3/2/2011 2:50 PM, Daniel D. Daugherty wrote:
> On 3/2/2011 11:25 AM, Coleen Phillimore wrote:
>> Summary: Redefine classes copies the constant pool while the constant 
>> pool may be resolving strings or classes
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/6512830/
>> bug link at http://bugs.sun.com/view_bug.do?bug_id=6512830
>
> src/share/vm/oops/constantPoolOop.cpp
>    So the race is that one thread is resolving something in
>    the CP we're copying. By fetching a copy of the CPSlot,
>    you're eliminating the data race. The copy you have is
>    either resolved or it's not resolved and now the call
>    to unresolved_klass_at_put() will see stable data...
>
>    Do I have this right? If so, then very nice catch!

Yes, exactly.
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
>    By adding JVM_CONSTANT_UnresolvedClass to this part of
>    the case switch, constantPoolOopDesc::copy_entry_to()
>    won't be called from RedefineClasses() so the change you
>    made in constantPoolOop.cpp for JVM_CONSTANT_UnresolvedClass
>    won't come into play for RedefineClasses(). Not a problem,
>    just an observation.
>
Good observation, it is not strictly needed because we handle it earlier 
in redefine classes but I made the change because I also changed 
JVM_CONSTANT_Unresolved string in a similar manner and thought it should 
be consistent.  The other callers seem to also be from redefine classes, 
just at different points, after the constant pool has been copied to a 
temporary though.  Is this right?

>    However that change will come into play for other callers...
>    And the change for JVM_CONSTANT_UnresolvedString will
>    come into play for RedefineClasses().

Yes, the UnresolvedString case is needed for RedefineClasses() for the 
same reason as the UnresolvedClass.  Apparently RedefineClasses doesn't 
unresolve the string as it does for the class for some reason that I 
don't know.
>
>    For RedefineClasses(), the classes need to be unresolved
>    at this stage of merging so your fix makes sure that even
>    a class that's in the middle of being resolved right now
>    stays unresolved.
>
Yes.
> Very nice! And thumbs up!
>
Thank you!!
Coleen
> Dan
>
>
>>
>> Tested with jvmti tests.
>>
>> Thanks,
>> Coleen
>>