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

[Coleen Phillimore]

Hi Serguei,

Thank you for looking at this change.

On 4/11/16 9:17 PM, serguei.spitsyn at oracle.com wrote:
> Coleen,
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
> - // Update the version number of the constant pool
> + // Update the version number of the constant pools (may keep scratch_cp)
>     merge_cp->increment_and_save_version(old_cp->version());
> + scratch_cp->increment_and_save_version(old_cp->version());
>
> Not sure, I understand the change above.
> Could you, please, explain why this change is needed?
> I suspect, the scratch_cp->version() is never used.

scratch_cp is used if it's equivalent to the old constant pool (see the 
code below this with the comments).  But it could add entries. In this 
case, we want scratch_cp to have a new version number because 
scratch_class->_source_file_name_index may be an appended entry 
(old_cp->length() + n) which a parallel constant pool merge might append 
a different entry and be set to the constant pool after the safepoint.   
So source_file_name_index won't point to the first appended entry.  So I 
need to update the version also in scratch_cp to detect this.

Actually, I made this change because I was going to make a bigger change 
that compared constant pool entries if they were the same version (ie 
both old_cp->version + 1), indicating parallel constant pool merging.  I 
decided this change was too much.

>
>
> + // NOTE: this doesn't work because you can redefine the same class 
> in two
> + // threads, each getting their own constant pool data appended to the
> + // original constant pool. In order for the new methods to work when 
> they
> + // become old methods, they need to keep their updated copy of the 
> constant pool.
> +
> It feels like the statement in this note is too strong, and as such, 
> confusing.
> Would it be better to tell something like "not always work"?
> Otherwise, the question is: why do we need this block of code if it 
> doesn't work?
>

The block of code is #if 0'ed out.   In my debugging I figured out why 
it wouldn't work, so I thought I'd comment it.

Thanks,
Coleen

>
> Thanks,
> Serguei
>
>
> On 4/11/16 13:06, 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
>> 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
>