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

[Daniel D. Daugherty]

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