617254 - Array changes to support ap-style arguments arrays

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Alex Macdonald]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: 

When transitioning from functions that require the ap-style convention to those that need the vm-style convention we need to parse a va_list and generate a dense Atom array suitable for use by an ArrayObject. I've pushed this logic inside ArrayClass/Object so that we can directly construct an ArrayObject given a cdecl argument descriptor and a va_list without exposing the inner working of ArrayObject to the rest of the codebase.

Reproducible: Always

Comment 1

[Alex Macdonald]

Attachment: Array changes

Comment 2

[Lars T Hansen]

This can't land until the exactgc changes have landed (sometime this week), but otherwise there shouldn't be any problem.  I will try to review it by early next week (big backlog).

Comment 3

[Steven Johnson]

Comment on attachment 495765 [details] [diff] [review]
Array changes

Rejecting only because I don't want to approve List::getRawPtr(), even ifdef AOT, unless it's shown that it's unacceptable to access the list entries via existing API. Otherwise looks OK.

If ArrayObject::getDenseCopy() is used only by AOT, it should be #ifdef FEATURE_AOT.

style nit: template args in Tamarin are generally all-caps, so

	template <typename adt>

would be better off as

	template <typename ADT>

style nit: preferred style in Tamarin is "if (cond)", not "if(cond)". Ditto "for ()" vs "for()".

Comment 4

[Alex Macdonald]

Attachment: Array Changes v2

fixed the stylistic bugs with the previous patch

Comment 5

[Alex Macdonald]

(In reply to comment #3)
> Rejecting only because I don't want to approve List::getRawPtr(), even ifdef
> AOT, unless it's shown that it's unacceptable to access the list entries via
> existing API.

It would require a re-write of a large part of the cdecl code which I'd like to avoid at this stage of the game. Another approach that would avoid the direct access to the underlying buffer would be if we could construct an ArrayObject with a given Atom* buffer, but I couldn't see a clean way of doing that...

Comment 6

[Alex Macdonald]

Attachment: Array Changes v3

missed a file in the updated patch

Comment 7

[Steven Johnson]

(In reply to comment #5)
> access to the underlying buffer would be if we could construct an ArrayObject
> with a given Atom* buffer, but I couldn't see a clean way of doing that...

There is such a way; see


    ArrayObject* ArrayClass::newarray(Atom* argv, int argc)

Comment 8

[Steven Johnson]

(In reply to comment #7)
> (In reply to comment #5)
> > access to the underlying buffer would be if we could construct an ArrayObject
> > with a given Atom* buffer, but I couldn't see a clean way of doing that...
> 
> There is such a way; see
>     ArrayObject* ArrayClass::newarray(Atom* argv, int argc)

Before I review the revised patch, could you take a look and see if this would solve your issue, allowing us to avoid getRawPtr()?

Comment 9

[Alex Macdonald]

(In reply to comment #8)
> (In reply to comment #7)
> > There is such a way; see
> >     ArrayObject* ArrayClass::newarray(Atom* argv, int argc)
> 
> Before I review the revised patch, could you take a look and see if this would
> solve your issue, allowing us to avoid getRawPtr()?

We're trying to avoid the unnecessary double allocation and copy, "newarray(Atom* argv, int argc)" copies the data out of the buffer rather than capturing the pointer

Comment 10

[Steven Johnson]

Wait, what? You want to alias the pointer from an Array? Sorry, I won't approve that.

Comment 11

[Alex Macdonald]

(In reply to comment #10)
> Wait, what? You want to alias the pointer from an Array? Sorry, I won't approve
> that.

no, no I don't want to do that. We need to convert a va_list into an ArrayObject, previously we would allocate an Atom* array parse the arg descriptor and put all the right atoms into this array, we would then pass this array to newArray(Atom*, int argc) so it could copy the data out and then we would delete our temporary array.

The proposed changes in this patch avoid this unnecessary allocation by creating an ArrayObject of the right size and then storing values directly into the backing store for the array object, ideally we'd do this with the exposed get/set operators but that would require a re-write of most of cdecl, instead we use the slightly ugly "getRawPtr()".

Comment 12

[Steven Johnson]

(In reply to comment #11)
> instead
> we use the slightly ugly "getRawPtr()".

The problem is not that it's slightly ugly; the problem is that it's allowing you to dance directly on the underlying storage of the List. This may happen to work correctly right now, but is 100% guaranteed to break in the future (in fact, I'll be quite surprised if this works properly post-ExactGC). The old List class had a way to peek directly at the data, and it was misused in various scary ways; I really don't want to re-open that door, even if this is specific for AOT.

Comment 13

[Alex Macdonald]

Attachment: Array Changes v4

now with 100% less getRawPtr and a lot more cdecl refactoring

Comment 14

[Lars T Hansen]

Is this patch also including the cdecl refactoring?

Comment 15

[Alex Macdonald]

Attachment: Array changes v5

Updated to take the recent GC work into account and I've moved the cdecl work back into its original patch

Comment 16

[Lars T Hansen]

Comment on attachment 499002 [details] [diff] [review]
Array changes v5

This seems fine.  You can optimize the write barrier in denseCopy() a hair if you are concerned about performance because you know that the existing values in the array are NULL, hence there's no need to check whether a decrementref is needed - use atomWriteBarrier_ctor instead of plain atomWriteBarrier.

The multiplication in the call to AllocPtrZero is only safe because we know you're copying out of a structure that's as large as the one you're copying into, but that's pretty subtle and in general this kind of length calculation is prone to result in security problems and is considered very bad style indeed.  Use Calloc() instead.  Suggest you change it when you land the patch.

Comment 17

[Alex Macdonald]

Attachment: Array changes v6

rebase to take into account recent changes

Comment 18

[Steven Johnson]

TR 5854:f33e20e92f91