Wasm: RhpNewArrayAlign8 use non-padded size when going to the slow path

[yowl]

This PR fixes a bug when allocating arrays that required 8 byte alignment, if the slow path was chosen the padded size, i.e. with the extra 12 byte object, was being passed to RhpGcAlloc (and RhpPublishObject). This change passes the size of the actual new array without the padding.

Fixes #8317

[yowl]

Think I may have the same bug in the other places where g_pFreeObjectEEType is used.

[yowl]

I've changed the other places where this occurs. I'm not convinced this is the whole story, firstly because I'm still seeing objects aligned at the 2 byte boundary when the GC runs which should be possible, right? Also I can't get any class to go through RhpNewFastAlign8.

[jkotas]

This can hit integer overflow and misbehave. There are other places in this method that can misbehave due to integer overflow. It would be nice to at least add TODO about it.

[yowl]

What would be the right behaviour here, to throw (OOM/Overflow)?

[yowl]

Or shall I just do something like this for #ifndef HOST_64BIT

        // Perform the size computation using 64-bit integeres to detect overflow
        uint64_t size64 = (uint64_t)baseSize + ((uint64_t)numElements * (uint64_t)pArrayEEType->get_ComponentSize());
        size64 = (size64 + (sizeof(UIntNative) - 1)) & ~(sizeof(UIntNative) - 1);

        size = (size_t)size64;
        if (size != size64)
        {
            ASSERT_UNCONDITIONALLY("NYI");  // TODO: Throw overflow
        }

?

[jkotas]

Yes, it would be nice to add ASSERT_UNCONDITIONALLY TODO for now that will be eventually replaced by throwing proper exception.

[yowl]

Think I've got every possible overflow covered now.

[jkotas]

RhpNewFastAlign8 does not make sense for 64-bit. And if it did, the constant 12 would make sense for it. I think you can ifdef out the whole method for 64-bit.

[jkotas]

It should be better and faster to write it like this:

BYTE *allocPtr = allocContext->alloc_ptr;
_ASSERTE(allocPtr <= allocContext->alloc_limit);

if ((SIZE_T)(acontext->alloc_limit - alloc_ptr) <= size)
{
    pObject = (Array *)allocPtr;
    acontext->alloc_ptr = >allocPtr + size;
    pObject->set_EEType(pArrayEEType);
    ...
}

It is the pattern used by CoreCLR allocation helpers: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/jithelpers.cpp . It is faster on 32-bit platforms because of 64-bit math is typically more than 2x slower than 32-bit math on 32-bit platforms.

Also, it works for both 32-bit and 64-bit. The overflow can technically happen on 64-bit platforms too.

[yowl]

Thanks, shall I do something similar for the alignment addition using SIZE_MAX. Also for x64 targets I'm using 12 as the size for g_pFreeObjectEEType I guess that is wrong?

[jkotas]

alignment addition using SIZE_MAX

I am not sure what you mean.

using 12 as the size for g_pFreeObjectEEType I guess that is wrong?

Right, size of g_pFreeObjectEEType on 64-bit platforms is 24. The Align8 methods do not make sense and won't be used on 64-bit platforms at all, so it is better to exclude them on 64-bit platforms.

[yowl]

alignment addition using SIZE_MAX

I am not sure what you mean.

For the alloc ptr we do something like (size_t)(acontext->alloc_limit - alloc_ptr) >= paddedSize to make sure there is space which together with the assert on alloc_ptr <= acontext->alloc_limit ensures its not going to overflow. When adding the 12 bytes to the size for the alignment, it could technically overflow as well and currently that is handled by using 64 bit arithmetic. Instead it could be if(size > SIZE_MAX -12) throw...

[jkotas]

Sounds good

[jkotas]

Thanks