Feeding TypedArray to js-ctypes

RESOLVED FIXED in mozilla18

Status

()

Core
js-ctypes
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({dev-doc-needed})

unspecified
mozilla18
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Bug 732936 lets users feed ArrayBuffer to js-ctypes.
We should do the same with TypedArray.
After toying with the idea, I tend to believe that we should accept to convert any typed array to any array/pointer type, without attempting to add a veneer of type-safety.
Created attachment 666181 [details] [diff] [review]
Feeding typed arrays to js-ctypes
Assignee: nobody → dteller
Attachment #666181 - Flags: review?(jorendorff)
Blocks: 771094
Comment on attachment 666181 [details] [diff] [review]
Feeding typed arrays to js-ctypes

Review of attachment 666181 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks correct. r=me with stricter implicit conversions.

::: js/src/ctypes/CTypes.cpp
@@ +2194,5 @@
> +
> +      // FIXME: Should we need to, we can obtain the size of an element in the
> +      // array as follows:
> +      // JS_GetTypedArrayByteLength(obj, cx)/JS_GetTypedArrayLength(obj, cx)
> +      *static_cast<void**>(buffer) = JS_GetArrayBufferViewData(valObj, cx);

Please check that the pointer type is consistent (or void *).

Users can easily use ctypes.cast if they really want a reinterpret_cast. Or just pass the ArrayBuffer, which is more like a void*.

@@ +2304,5 @@
> +      // Check that array is consistent with type, then
> +      // copy the array. As with C arrays, data is *not*
> +      // copied back to the ArrayBuffer at the end of a
> +      // function call, so do not expect this to work
> +      // as an inout argument.

ISTR arguments of array type are silently converted to pointers, because that's actually what C does! These two declarations are identical in every way:

    void f(int x[]);
    void f(int *x);

So I think a TypedArray actually *will* work as an in-out argument to such a function, because this particular code won't get called--the pointer code above runs instead.

@@ +2309,5 @@
> +      uint32_t sourceLength = JS_GetTypedArrayByteLength(valObj, cx);
> +      size_t elementSize = CType::GetSize(baseType);
> +      size_t arraySize = elementSize * targetLength;
> +      if (arraySize != size_t(sourceLength)) {
> +        JS_ReportError(cx, "typed array length does not match source array length");

Please check that the types really match, not just that the length is the same. It should be pretty easy; treat a Uint32Array just like a uint32_t.array(length).

::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
@@ +1765,5 @@
> +         [new Uint32Array(c_arraybuffer), ctypes.uint32_t],
> +         [new Float32Array(c_arraybuffer), ctypes.float32_t],
> +         [new Float64Array(c_arraybuffer), ctypes.float64_t]
> +       ]) {
> +    let [view, item_type] = sample;

For what it's worth, you can say 'for (let [view, item_type] of ...)'
Attachment #666181 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> ::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
> @@ +1765,5 @@
> > +         [new Uint32Array(c_arraybuffer), ctypes.uint32_t],
> > +         [new Float32Array(c_arraybuffer), ctypes.float32_t],
> > +         [new Float64Array(c_arraybuffer), ctypes.float64_t]
> > +       ]) {
> > +    let [view, item_type] = sample;
> 
> For what it's worth, you can say 'for (let [view, item_type] of ...)'

Good point.
Created attachment 667403 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v2

Attaching a new version that performs strict type-checking.
Note that we cannot convert a Int32Array to a int[] or int* even on a platform on which int is 32 bits. I believe that allowing this would be a misfeature, what do you think?
Attachment #666181 - Attachment is obsolete: true
Attachment #667403 - Flags: review?(jorendorff)
Comment on attachment 667403 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v2

Review of attachment 667403 [details] [diff] [review]:
-----------------------------------------------------------------

It still doesn't check for exact type match when converting a typedarray to a ctypes array. r=me with that.

::: js/src/ctypes/CTypes.cpp
@@ +2202,5 @@
> +        TypeCode elementTypeCode;
> +        switch (JS_GetTypedArrayType(valObj, cx)) {
> +        case TypedArray::TYPE_INT8:
> +          elementTypeCode = TYPE_int8_t;
> +          break;

Factor this code into a function and use the same function in the array case.
Attachment #667403 - Flags: review?(jorendorff) → review+
(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Attaching a new version that performs strict type-checking.
> Note that we cannot convert a Int32Array to a int[] or int* even on a
> platform on which int is 32 bits. I believe that allowing this would be a
> misfeature, what do you think?

It'd be inconsistent with the existing design. I'm ok with revisiting that design decision later.
Created attachment 667531 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v3

Ah, my bad, forgot to check type-conversion with arrays.
Attaching a new version. I took the opportunity to improve testing of ArrayBuffer/typed array to array conversions.
Attachment #667403 - Attachment is obsolete: true
Attachment #667531 - Flags: review?(jorendorff)
Created attachment 667577 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v3

Same patch, *after* qref.
Attachment #667531 - Attachment is obsolete: true
Attachment #667531 - Flags: review?(jorendorff)
Attachment #667577 - Flags: review?(jorendorff)
Attachment #667577 - Flags: review?(jorendorff) → review+
Created attachment 668435 [details] [diff] [review]
2. More tests

I just realized that my previous tests fail to cover the critical case in which the typed array does not start at offset 0 of the buffer. Here are the additional tests.
Attachment #668435 - Flags: review?(jorendorff)
Comment on attachment 668435 [details] [diff] [review]
2. More tests

Gold star.
Attachment #668435 - Flags: review?(jorendorff) → review+
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be4e7680b42
https://hg.mozilla.org/integration/mozilla-inbound/rev/03e4c6401c35
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7be4e7680b42
https://hg.mozilla.org/mozilla-central/rev/03e4c6401c35
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.