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

[Daniel D. Daugherty]

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.

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.

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

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