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

[serguei.spitsyn at oracle.com]

On 4/12/16 12:29, Coleen Phillimore wrote:
>
> 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.

Thank you for the explanation.
I'm Ok with this change, just wanted to understand.

I think, we have to prevent multiple class redefinitions (prologues) of 
the same class at the same time.
Otherwise, it is hard to isolate and fix all potential issues in this 
scenario.
I doubt, the original goal was to allow this.

I've never investigated this corner case.
It is not clear what happens with two merged constant pools prepared 
concurrently.
We do not merge them again, right?
Most likely,  the last redefinition wins with some side effects.
If so, then there has to be a way to detect and prevent this kind of 
concurrency.

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

Oh, I see.
In this particular case, I looked at the Udiff that does not show all 
the context.

Please, consider it reviewed.

Thanks,
Serguei


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