RFR 8151546: nsk/jvmti/RedefineClasses/StressRedefine fails in hs nightly

[serguei.spitsyn at oracle.com]

On 4/13/16 09:48, Coleen Phillimore wrote:
>
> Dan, Thank you for reviewing this.
>
> On 4/13/16 12:07 PM, Daniel D. Daugherty wrote:
>> On 4/11/16 2:06 PM, Coleen Phillimore wrote:
>>> Summary: Constant pool merging is not thread safe for source_file_name.
>>>
>>> This change includes the change for the following bug because they 
>>> are tested together.
>>>
>>> 8148772: VM crash in nsk/jvmti/RedefineClasses/StressRedefine: 
>>> assert failed: Corrupted constant pool
>>> Summary: ConstantPool::resolve_constant_at_impl() isn't thread safe 
>>> for MethodHandleInError and MethodTypeInError.
>>>
>>> The parallel constant pool merges are mostly harmless because the 
>>> old methods constant pool pointers aren't updated.  The only case I 
>>> found where it isn't harmless is that we rely on finding the 
>>> source_file_name_index from the final merged constant pool, which 
>>> could be any of the parallel merged constant pools.  The code to 
>>> attempt to dig out the name from redefined classes is removed.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8151546.01/webrev
>>
>> src/share/vm/classfile/javaClasses.inline.hpp
>>     Perhaps instead of this comment:
>>
>>     L226:   // Because constant pools can be merged in parallel, the 
>> source file name index
>>     L227:   // may be merged over with something else in a previous 
>> version.
>>
>>     please consider this one:
>>
>>     // RedefineClasses() currently permits redefine operations to
>>     // happen in parallel using a "last one wins" philosophy. That
>>     // spec laxness allows the constant pool entry associated with
>>     // the source_file_name_index for any older constant pool version
>>     // to be unstable so we shouldn't try to use it.
>
> Okay, your comment is more complete.  I'll use your comment.
>>
>> src/share/vm/oops/constantPool.hpp
>>     I think this file is all changes from 8148772 which I've already
>>     reviewed so no comments.
>>
>> src/share/vm/oops/constantPool.cpp
>>     I think this file is all changes from 8148772 which I've already
>>     reviewed so no comments.
>>
>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>>     The version number in the constant pool is new to my brain since
>>     I last dove into this code in detail...
>>
>>     Right now you do the version increment here:
>>
>>     L1445:   // Update the version number of the constant pools (may 
>> keep scratch_cp)
>>     L1446: merge_cp->increment_and_save_version(old_cp->version());
>>     L1447: scratch_cp->increment_and_save_version(old_cp->version());
>>
>>     You could choose to only do it when you know that you're going
>>     to keep the scratch_cp, but maybe that's being too picky.
>>
>
> I'd have to set it in two places if I did that, so I picked just 
> once.  If we use the merged_cp, the scratch_cp is discarded so the 
> version doesn't matter.
>
>> Serguei's find of this very old bug is good:
>>
>> JDK-6227506 JVMTI Spec: Atomicity of RedefineClasses should be specified
>> https://bugs.openjdk.java.net/browse/JDK-6227506
>>
>> There's another bug out there will all the notes that Tim Bell
>> took when we did the monster RedefineClasses code walk through.
>> I believe in that bug, the lack of locking/atomicity was also
>> called out. I'll see if I can find that bug...
>>
>
> Yes, that would be good.   There are a lot of statics in 
> VM_RedefineClasses.

>   I don't know why these don't cause bugs!

I think, they do but very rarely and intermittently.
This is rare corner case when two agents or two threads of one agent do 
concurrent redefinitions.

Thanks,
Serguei

>
> Coleen
>
>> Dan
>>
>>
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8151546
>>>
>>> Tested with rbt, java/lang/instrument tests, com/sun/jdi tests. I 
>>> tried to write a test with all the conditions of the failure but 
>>> couldn't make it fail (so noreg-hard).
>>>
>>> Thanks,
>>> Coleen
>>
>