Request for review (S) 6512830: Error: assert(tag_at(which).is_unresolved_klass(), "Corrupted constant,pool")
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Mar 3 08:52:27 PST 2011
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
>> 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