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

[Coleen Phillimore]

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!

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
>