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

Coleen Phillimore coleen.phillimore at oracle.com
Thu Mar 3 09:11:25 PST 2011


Yes, Dan, you are _still_ the RedefineClasses() expert!  Thanks for the 
code review.

Coleen

(resending after bounce)

On 3/3/2011 11:52 AM, Daniel D. Daugherty wrote:
> On 3/3/2011 9:28 AM, Coleen Phillimore wrote:
>> 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.
>
> Agreed. Consistency is good...
>
>
>> 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?
>
> Yes, if by temporary you mean the merged CP.
>
>
>>
>>>    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.
>
> RedefineClasses() unresolves the classes because the verifier was
> unhappy when it was given resolved classes. The verifier didn't
> complain about resolved Strings so I didn't unresolve them.
>
> I guess it's too late to claim that I know absolutely nothing about
> RedefineClasses()? :-)
>
> Dan
>
>
>
>>>
>>>    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
>>>>
>>



More information about the hotspot-runtime-dev mailing list