RFR(S): 8155241: Crash with assert in Xcomp mode and with disabled ReduceBulkZeroing

[Tobias Hartmann]

Thanks, Vladimir!

Best regards,
Tobias

On 11.05.2016 09:23, Vladimir Kozlov wrote:
> Okay. Good. Reviewed.
> 
> Thanks,
> Vladimir
> 
> On 5/11/16 12:07 AM, Tobias Hartmann wrote:
>> Thanks, Roland and Vladimir!
>>
>> On 10.05.2016 22:49, Vladimir Kozlov wrote:
>>> My concern is that the code which calls find_previous_arraycopy() may have dependency on object initialization by arracopy. It, for example, can assume that whole allocated object is initialized by so we can load value from arraycopy's src array. Why the assert checks that allocatoin is initialized by arraycopy otherwise?
>>
>> Yes, I was first concerned about this as well but looking at the code, I don't think it's a problem:
>> LoadNode::find_previous_arraycopy() searches for an arraycopy that writes to the same memory that is loaded by 'this'. The assert checks the CloneBasic case, which always initializes the whole object anyway.
>>
>> As Roland said, I think this assert is only a sanity check that is unrelated to the usage of find_previous_arraycopy(). It's safe to return a CloneBasic because it's guaranteed to initialize the whole object.
>>
>>> On other hand set_clonebasic() is only used in Object.clone() intrinsic where whole object is copied by arraycopy. So initialization complete check is useless unless set_clonebasic() could be used in other cases.
>>
>> Yes, with CloneBasic we always initialize the whole object and with ReduceBulkZeroing enabled, we set_complete_with_arraycopy() in LibraryCallKit::copy_to_clone() to avoid the useless zeroing. If ReduceBulkZeroing is disabled, we first zero the object and then overwrite it again. CloneBasic shouldn't be used in other cases.
>>
>> RBT testing showed no problems.
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 5/10/16 11:49 AM, Roland Westrelin wrote:
>>>>
>>>>> Yes, I think the assert is wrong because it assumes that a clonebasic
>>>>> arraycopy always takes care of zeroing (simply by overwriting) a
>>>>> newly allocated destination array. However, with
>>>>> -XX:-ReduceBulkZeroing this is not the case and the array is
>>>>> needlessly zeroed directly after the allocation and before the
>>>>> clonebasic which overwrites the array anyway.
>>>>>
>>>>> Maybe Roland (CC'ed) can verify.
>>>>
>>>> I don't remember for sure but AFAICT, the assert is only a sanity check
>>>> so I would say the change is good.
>>>>
>>>> Roland.
>>>>