517856 - Bugs and performance traps in new allocation code

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

Description

[Lars T Hansen]

The code in GCGlobalNew appears to be wrong in several ways.

MMgcNewArrayCall adds space for a size_t field at the beginning of the object and adjusts the start of the first element past this field.  That is wrong if it means that the first element is not aligned on a natural boundary for the largest primitive datum the element's type contains.  In particular, it is wrong if the first element contains a 'double'.  We get compilation errors on ARM because of this (a warning); it is known that MIPS requires 8-byte alignment for doubles, and it is suspected that PPC might require it.

The same function checks whether the call to NewArrayCalloc returns NULL and returns NULL if so, but it also checks whether the kCanFail flag is set.  The latter check is redundant, as the lower layer should never return NULL if that flag is not set.

MMgcNewArrayCall is very large for an inline function, especially considering that it's always calling out of line to an allocator function anyway.  The better strategy would be to have simple inline code that calls out to a function that performs more work in the body of the function itself.

NewArrayCalloc /severely/ violates its contract by returning NULL on overflow even if the kCanFail flag is not set.

Comment 1

[Steven Johnson]

(In reply to comment #0)
> In particular, it is wrong if the first element contains a 'double'.

IIRC, x86 doesn't *require* 8-byte alignment, but there can be a load penalty if it's not... I think we encountered this during Box work and accidentally 4-aligned the interpreter stack (rather than 8-aligned).

Comment 2

[Lars T Hansen]

Data point: GCC on MacOS X does not bloat a 12-byte structure to 16 bytes to force alignment; an array of 10 such elements is still 120 bytes long.

Comment 3

[Lars T Hansen]

From Antti: 

"We have specialized versions of the MMgcNewArrayCall template for the basic primitive types, which do not allocate the extra size_t for storing the array element count. ... The types that currently have this optimization are: uint8_t, char, uint16_t, int16_t, uint32_t, int32_t, uint64_t, int64_t, void*, void**.
 
We should probably add double (and float) to the list of types to be handled this way."

We should definitely do that.

We should also worry about pointer types of all kinds, if those are not handled by void* (doesn't appear that way).  And we should worry about whether compilers treat 'int' and 'int32_t' the same on suitable platforms, or if those are distinguished at the level of template specialization and instantiation.

Comment 4

[Lars T Hansen]

The debugging cookie also needs an aligned header.  On a 32-bit system we could use the normal 8-byte header for both the array length (first word) and debugging cookie (second word), but I don't know if it's worth the bother.

Comment 5

[Lars T Hansen]

Attachment: Patch, v1

This is a big patch (unfortunately) and it does the following:

- The array header and debug tag are now uniformly aligned on 8-byte 
  boundaries
- NULL is only returned if kCanFail was specified (part of the contract)
- If the object size exceeds the system limit then a callout is made to a
  handler in GCHeap; that handler will never return.  The handler is part
  of this patch
- I added cases for 'double' and 'float' primitive arrays
- Some code has moved out of the inline functions and into a lower level.
  If there are efficiency concerns about this we can split NewTaggedArray
  into two, one for the primitive case and one for the non-primitive case,
  but I don't see it as a big deal.
- I have commented a lot.
- I have renamed a lot of functions because their names made scant sense to me
  and were - to my mind - occasionally misleading.  Arguably more could be
  done but I decided to stop for the time being.
- I also moved to more uniform use of vector vs array, preferring the latter
  name pretty much everywhere.

If there's too much here let me know and I can try to split the patch.

Comment 6

[Lars T Hansen]

Note the size calculation may wraparound on 64-bit systems (with a 64-bit size_t) and we need to guard against that in the normal manner.

Comment 7

[Rishit Shah]

Comment on attachment 403455 [details] [diff] [review]
Patch, v1


- In NewTaggedArray()
void *p = TaggedAlloc((size_t)size64, opts, MMGC_NORM_ARRAY_GUARD + isPrimitive);

Some compilers might warn against bool to integer type conversion for isPrimitive.

Same for IsArrayAllocation() though its probably debug only.

- SignalObjectTooLarge() calls VMPI_exit() that will abort the process.  Shouldn't we be invoking GCHeap::Abort().  If flash content is trying to allocate unreasonable sized object we shouldn't cause the browser process to exit.

Comment 8

[Lars T Hansen]

(In reply to comment #7)
> (From update of attachment 403455 [details] [diff] [review])
> 
> - In NewTaggedArray()
> void *p = TaggedAlloc((size_t)size64, opts, MMGC_NORM_ARRAY_GUARD +
> isPrimitive);
> 
> Some compilers might warn against bool to integer type conversion for
> isPrimitive.

Fixed.

> Same for IsArrayAllocation() though its probably debug only.

Fixed.

> - SignalObjectTooLarge() calls VMPI_exit() that will abort the process. 
> Shouldn't we be invoking GCHeap::Abort().  If flash content is trying to
> allocate unreasonable sized object we shouldn't cause the browser process to
> exit.

I agree.  Please continue this discussion on bug #519283.

Comment 9

[Lars T Hansen]

redux changeset:   2651:b73b66c0dce0.  The guard for size calculation wraparound will land separately.