[vmkit-commits] [vmkit] r145527 - /vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc

[Will Dietz]

On Wed, Nov 30, 2011 at 4:45 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
> Hi Will,
>
> On Wed, Nov 30, 2011 at 11:12 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>
>> Author: wdietz2
>> Date: Wed Nov 30 16:12:56 2011
>> New Revision: 145527
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=145527&view=rev
>> Log:
>> ArrayCopy: handle overlapping arrays of Objects
>>
>> Modified:
>>    vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc
>>
>> Modified: vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc
>> URL:
>> http://llvm.org/viewvc/llvm-project/vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc?rev=145527&r1=145526&r2=145527&view=diff
>>
>> ==============================================================================
>> --- vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc (original)
>> +++ vmkit/trunk/lib/j3/ClassLib/ArrayCopy.inc Wed Nov 30 16:12:56 2011
>> @@ -62,17 +62,30 @@
>>   }
>>
>>   if (!(dstType->isPrimitive())) {
>> +    // Scan to ensure element compatibility, recording the first item
>> +    // that requires an exception be thrown.
>> +    int badEl = -1;
>>     for (int i = 0; i < len; i++) {
>>       cur = ArrayObject::getElement((ArrayObject*)src, i + sstart);
>>       if (cur) {
>>         if (!(JavaObject::getClass(cur)->isAssignableFrom(dstType))) {
>> -          th->throwException(vm->CreateArrayStoreException(
>> -              (JavaVirtualTable*)dst->getVirtualTable()));
>> -          UNREACHABLE();
>> +          badEl = i;
>> +          break;
>>         }
>>       }
>> -      ArrayObject::setElement((ArrayObject*)dst, cur, i + dstart);
>>     }
>> +
>> +    // Copy over the non-conflicting elements, handling overlaps
>> +    void* ptrDst =
>> (void*)((int64_t)ArrayObject::getElements((ArrayObject*)dst) +
>> dstart*sizeof(void*));
>> +    void* ptrSrc =
>> (void*)((int64_t)ArrayObject::getElements((ArrayObject*)src) +
>> sstart*sizeof(void*));
>> +    int copyLen = badEl == -1 ? len : badEl - 1;
>> +    if (copyLen > 0)
>> +      memmove(ptrDst, ptrSrc, copyLen * sizeof(void*));
>
>
> Please do not do a memmove on ArrayObject. It is fine to do a memmove on
> primitive arrays, but definitely not on arrays of objects. You need to
> execute a write barrier for each copied element.
>
> To fix overlaps, I guess you need to check the start indices for both
> arrays, and see which one starts first to know in which side you need to
> iterate (from end to start or from start to end).
>
> Nicolas

Okay, sure thing.  I mistakenly thought we didn't have write barriers
in the existing version for some reason, probably was looking at wrong
part of JavaArray.h, whoops.

Anyway, sorry about this and I'll either revert or fix this in mainline shortly.

Thanks!

~Will