Request for review (S) 6512830: Error: assert(tag_at(which).is_unresolved_klass(), "Corrupted constant,pool")
coleen.phillimore at oracle.com
Thu Mar 3 09:11:25 PST 2011
Yes, Dan, you are _still_ the RedefineClasses() expert! Thanks for the
(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
>>> 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.
>>> 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()? :-)
>>> 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.
>>> Very nice! And thumbs up!
>> Thank you!!
>>>> Tested with jvmti tests.
More information about the hotspot-runtime-dev