550351 - don't abort recording when accessing out of range typed array element

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The current code aborts tracing, which is bad; it should mimic what the interpreter does, which is to read undefined.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: don't abort on out of bounds

Don't abort on out of bounds, as per bug.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, also fixed previous non-uses of INS_VOID as well.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/0923925b4b8f

Comment 4

[Serge Gautherie (:sgautherie)]

Test fails on MacOSX SM+TB:
{
TEST-UNEXPECTED-FAIL | trace-test.py | /builds/slave/comm-central-trunk-macosx-debug/build/mozilla/js/src/trace-test/tests/basic/testTypedArrays.js: /builds/slave/comm-central-trunk-macosx-debug/build/mozilla/js/src/trace-test/tests/basic/testTypedArrays.js:21: Error: Assertion failed: got 4294968048, expected 752
}

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Test fails on MacOSX SM+TB:

FF too.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

... why does this fail only on the Mac? :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

fwiw, I see 4294968048 on that testcase with and without jit before and after this patch.  On Mac, that is.  On Linux I get an out-of-bounds exception on the

  ar[i-2] = ar[i+2];

line instead.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Though that might also be 32-bit (mac) vs 64-bit (linux) differences.

Comment 10

[Boris Zbarsky [:bzbarsky]]

On Mac, ar[12] and ar[13] are 0x80000000 after that "out of bounds" loop.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ok, linux exception is because that build doesn't have a fix for bug 534467 (the #if 0 that fix added, in particular).

Comment 12

[Boris Zbarsky [:bzbarsky]]

            jsdouble d;
            if (JS_ValueToNumber(cx, *vp, &d))
                tarray->setIndex(index, NativeType(d));

In my case on mac, JS_ValueToNumber returns true and |d| is:

(gdb) p d
$2 = nan(0xfffffffffffff)

Comment 13

[Boris Zbarsky [:bzbarsky]]

And uint32(d) seems to end up 0x80000000.

Comment 14

[Boris Zbarsky [:bzbarsky]]

#include <stdio.h>
int main() {
  double d;
  *(unsigned long long *)&d = 0xffffffffffffffffULL;
  printf("%f, 0x%x\n", d, (unsigned int)d);
  return 1;
}

On Linux: -nan, 0
On Mac: 0x80000000

On Linux, the relevant cast seems to be:

  cvttsd2siq	%xmm0, %rsi

(which claims that it should output 0x80000000 if overflowing, but apparently NaN is not considered overflow?).

On Mac, the assembly seems to be a lot longer, and in particular seems to be explicitly checking for NaN (e.g. cmplesd two values that are the same double value is involved).

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm.  So cvtsd2si putting in 0x80000000 seems bogus to me, or at least not something that we want to specify -- LIR_f2i on x86 just does that.  On ARM, I'm not sure -what- VCVT does in this case... the armv7 manual seems to be very vague.

This tells me that we probably need to make the f2i code a little more complicated in order to be able to rely on it.  For example, do we want to say:

NaN -> 0
+Inf -> maxint
-Inf -> minint

?

Or do we just want to leave it as-is, and have NaNs etc. be converted to integers in whatever platform specific way that happens?  If so, I can try to change the test so it doesn't rely on any particular conversion.

Comment 16

[Andreas Gal :gal]

f2i only produces a valid result if f is an integer. Its rounding modes are not supposed to be defined precisely. Doing that can be very expensive on some platforms.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #12)
>             jsdouble d;
>             if (JS_ValueToNumber(cx, *vp, &d))
>                 tarray->setIndex(index, NativeType(d));

Vlad, sorry if I missed this reviewing. You are eating an exception by ignoring a false return from JS_ValueToNumber. You want 

              if (!JS_ValueToNumber(cx, *vp, &d))
                  return false;

instead. Also, I think I did catch this in the initial view: use internal API where possible, not JS_* public API. In this case, js_ValueToNumber.

rs=me on fixing these issues.

/be

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Or do we just want to leave it as-is, and have NaNs etc. be converted to
> integers in whatever platform specific way that happens?

We do not want to slow down f2i if we can help it, but we even more do not want to have platform-dependent NaN-to-int conversions in JS. NaN-to-int is well defined by ECMA-262, witness:

js> 0/0
NaN
js> (0/0)>>0
0
js> (0/0)>>>0
0

/be

Comment 19

[Andreas Gal :gal]

f2i is not meant to be used this way. Its meant to be used for recovering integers from a float representation, so use it with i2f(f2i(f)) == f.

Comment 20

[Andreas Gal :gal]

If you need properly rounding double to integer conversion, use ECMAToInt32. f2i can be used to bypass it on the fast path if f is already an integer, so

if (i2f(i = f2i(f)) == f)
    return i;
else
    return ECMAToInt32(f)

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, ok, so it sounds like I need to use ECMAToInt32, and on trace to call the builtin; will the builtin get filtered out, or do I need to set up the construct described in comment #20?  If so, that could hurt because of the CSE issues across branches..

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> If you need properly rounding double to integer conversion, use ECMAToInt32.
> f2i can be used to bypass it on the fast path if f is already an integer, so
> 
> if (i2f(i = f2i(f)) == f)
>     return i;
> else
>     return ECMAToInt32(f)

Without the else after return of course :-P.

(In reply to comment #21)
> Hmm, ok, so it sounds like I need to use ECMAToInt32, and on trace to call the
> builtin; will the builtin get filtered out, or do I need to set up the
> construct described in comment #20?  If so, that could hurt because of the CSE
> issues across branches..

You need something like what is in comment 20.

We could guard on the value being finite and branch-exit to handle NaN, maybe.

/be

Comment 23

[Boris Zbarsky [:bzbarsky]]

branch-exit would be a bit of a pain in the seam-carving testcase, since it would lead to us creating 4-5 different traces, as far as I can tell, of which the last one created (and the slowest to execute, therefore) would be the hot one...

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: better patch

Better patch.  Fixes behaviour for out of bounds, as well as tightens behaviour for Nan/Inf and non-numeric values (NaN/Inf are preserved for floating-point arrays, otherwise they all become 0).

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: better patch, v2

An even better patch; passes tryserver and has better tests.

Behaviour is this:

- For integer arrays, Nan/Inf/etc. are assigned as 0
- For all arrays, out-of-bounds indexes are read as undefined
- Out-of-bounds assignment is ignored
- String assignment causes string-to-number conversion
- Object assignment is equivalent to NaN
- Undefined/boolean assignment is the same as BooleanOrUndefinedToNumber.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> - Out-of-bounds assignment is ignored

Except on trace where it OVERFLOW_EXITs, right?

Comment 27

[Boris Zbarsky [:bzbarsky]]

+++ b/js/src/jstracer.cpp
@@ -12022,80 +12022,84 @@ TraceRecorder::setElem(int lval_spindex,
+        // If it's not a number, convert objects to NaN,
+        // null to 0, and call StringToNumber or BooleanOrUndefinedToNumber
+        // for those.

"those"?  You mean other primitives?

Why do we want to convert objects to NaN instead of whatever the right valueOf thing is?

+                v_ins = lir->insImmf(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));

Why not just use |v_ins = lir->insImmf(js_NaN)| here?  Should be the same thing, without the masking and dereferencing.

+        switch (tarray->type) {
+          case js::TypedArray::TYPE_INT8:
+          case js::TypedArray::TYPE_INT16:
+          case js::TypedArray::TYPE_INT32:
+            v_ins = f2i(v_ins);

So these will still behave inconsistently on different OSes/compilers/whatever?  I'm not quite folowing why this is desirable.

+          case js::TypedArray::TYPE_UINT8:
+          case js::TypedArray::TYPE_UINT16:
+          case js::TypedArray::TYPE_UINT32:
+            v_ins = f2u(v_ins);

I would assume similar here...

+++ b/js/src/jstypedarray.cpp

+            // non-primitive assignments become NaN or 0 (for float/int arrays)
+            d = *JSVAL_TO_DOUBLE(cx->runtime->NaNValue);

If we cared about avoiding the double-to-int conversions in the int cases here, we could probably use the NumberTraits stuff we added recently to produce 0 at compile-time for ints.  Heck, we could use StringToNumberType for the JSString case here, for that matter, instead of calling js_ValueToNumber which will do all the type checks again.

And again, just use js_NaN here (and in the JSVAL_VOID case).

But why don't we need those valueOf conversions?

+        } else if (ArrayTypeIsUnsigned()) {
+            JS_ASSERT(sizeof(NativeType) <= 4);
+            uint32 n = js_DoubleToECMAUint32(d);
+            tarray->setIndex(index, NativeType(n));

Maybe also worth doing via NumberTraits.

+        } else if (ArrayTypeID() == TypedArray::TYPE_UINT8_CLAMPED) {
+            // The uint8_clamped type has a special rounding converter
+            // for doubles.
+            tarray->setIndex(index, NativeType(d));

Yes, but don't you then need to pass in |d| instead of |NativeType(d)| here?  And have a test for this?

+            JS_ASSERT(sizeof(NativeType) <= 4);
+            int32 n = js_DoubleToECMAInt32(d);
+            tarray->setIndex(index, NativeType(n));

Again, NumberTraits.

@@ -940,11 +990,15 @@ class TypedArrayTemplate
                     *dest++ = NativeType(JSVAL_TO_INT(v));
                 } else if (JSVAL_IS_DOUBLE(v)) {
                     *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
+                } else if (JSVAL_IS_PRIMITIVE(v)) {
+                    jsdouble dval = js_ValueToNumber(cx, &v);
+                    *dest++ = NativeType(dval);
                 } else {
+                    if (ArrayTypeIsFloatingPoint()) {
+                        *dest++ = NativeType(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));
+                    } else {
+                        *dest++ = NativeType(int32(0));
+                    }

This code is not consistent with the setter code for JSVAL_VOID, unless js_ValueToNumber turns undefined into NaN, in which case why do we need the setter case for JSVAL_VOID?  And again, why are we not doing proper valueOf conversions?

In case it's not clear why I'm harping on this valueOf business, should sticking a Date into an integer array work?  All other operations treating a Date as an integer work...

And again, NumberTraits can probably be used to simplify this code...


@@ -959,11 +1013,15 @@ class TypedArrayTemplate
                     *dest++ = NativeType(JSVAL_TO_INT(v));
                 } else if (JSVAL_IS_DOUBLE(v)) {
                     *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
+                } else if (JSVAL_IS_PRIMITIVE(v)) {
+                    jsdouble dval = js_ValueToNumber(cx, &v);
+                    *dest++ = NativeType(dval);
                 } else {
+                    if (ArrayTypeIsFloatingPoint()) {
+                        *dest++ = NativeType(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));
+                    } else {
+                        *dest++ = NativeType(int32(0));
+                    }

Inline method, macro, _something_ other than copy/paste, please!

I didn't look at the tests carefully so far.

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #27)
> +++ b/js/src/jstracer.cpp
> @@ -12022,80 +12022,84 @@ TraceRecorder::setElem(int lval_spindex,
> +        // If it's not a number, convert objects to NaN,
> +        // null to 0, and call StringToNumber or BooleanOrUndefinedToNumber
> +        // for those.
> 
> "those"?  You mean other primitives?

Right.

> Why do we want to convert objects to NaN instead of whatever the right valueOf
> thing is?

Talking with andreas it didn't sound desirable, because of type instability there... I could be convinced otherwise.

> +                v_ins = lir->insImmf(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));
> 
> Why not just use |v_ins = lir->insImmf(js_NaN)| here?  Should be the same
> thing, without the masking and dereferencing.

Because neither andreas nor I knew about/could find js_NaN :-)

> +        switch (tarray->type) {
> +          case js::TypedArray::TYPE_INT8:
> +          case js::TypedArray::TYPE_INT16:
> +          case js::TypedArray::TYPE_INT32:
> +            v_ins = f2i(v_ins);
> 
> So these will still behave inconsistently on different OSes/compilers/whatever?
>  I'm not quite folowing why this is desirable.

Nope, the f2i function isn't LIR_f2i; it does some optimizations and ultimately calls DoubleToECMAInt32.

> +          case js::TypedArray::TYPE_UINT8:
> +          case js::TypedArray::TYPE_UINT16:
> +          case js::TypedArray::TYPE_UINT32:
> +            v_ins = f2u(v_ins);
> 
> I would assume similar here...

Same thing here.

> +++ b/js/src/jstypedarray.cpp
> 
> +            // non-primitive assignments become NaN or 0 (for float/int
> arrays)
> +            d = *JSVAL_TO_DOUBLE(cx->runtime->NaNValue);
> 
> If we cared about avoiding the double-to-int conversions in the int cases here,
> we could probably use the NumberTraits stuff we added recently to produce 0 at
> compile-time for ints.  Heck, we could use StringToNumberType for the JSString
> case here, for that matter, instead of calling js_ValueToNumber which will do
> all the type checks again.

If the value to store is an int, it should already be handled in the if block above this one, where all this work is short-circuited.  If the value is a double, then we have to do the conversion anyway, so I don't think we can specialize anything further at compile time.

> And again, just use js_NaN here (and in the JSVAL_VOID case).
> 
> But why don't we need those valueOf conversions?
> 
> +        } else if (ArrayTypeIsUnsigned()) {
> +            JS_ASSERT(sizeof(NativeType) <= 4);
> +            uint32 n = js_DoubleToECMAUint32(d);
> +            tarray->setIndex(index, NativeType(n));
> 
> Maybe also worth doing via NumberTraits.

Not familiar with NumberTraits -- will look into it.

> +        } else if (ArrayTypeID() == TypedArray::TYPE_UINT8_CLAMPED) {
> +            // The uint8_clamped type has a special rounding converter
> +            // for doubles.
> +            tarray->setIndex(index, NativeType(d));
> 
> Yes, but don't you then need to pass in |d| instead of |NativeType(d)| here? 
> And have a test for this?

Well, NativeType(d) -> uint8_clamped(d) which will invoke the rounding double constructor/converter.  setIndex does take a NativeType, so maybe the cast/construction is redundant, but it seems to make some compilers happier.

> +            JS_ASSERT(sizeof(NativeType) <= 4);
> +            int32 n = js_DoubleToECMAInt32(d);
> +            tarray->setIndex(index, NativeType(n));
> 
> Again, NumberTraits.
> 
> @@ -940,11 +990,15 @@ class TypedArrayTemplate
>                      *dest++ = NativeType(JSVAL_TO_INT(v));
>                  } else if (JSVAL_IS_DOUBLE(v)) {
>                      *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
> +                } else if (JSVAL_IS_PRIMITIVE(v)) {
> +                    jsdouble dval = js_ValueToNumber(cx, &v);
> +                    *dest++ = NativeType(dval);
>                  } else {
> +                    if (ArrayTypeIsFloatingPoint()) {
> +                        *dest++ =
> NativeType(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));
> +                    } else {
> +                        *dest++ = NativeType(int32(0));
> +                    }
> 
> This code is not consistent with the setter code for JSVAL_VOID, unless
> js_ValueToNumber turns undefined into NaN, in which case why do we need the
> setter case for JSVAL_VOID?  And again, why are we not doing proper valueOf
> conversions?
> 
> In case it's not clear why I'm harping on this valueOf business, should
> sticking a Date into an integer array work?  All other operations treating a
> Date as an integer work...

Well, like I said, concern over type stability, but then again if we're already doing a bunch of branches here and there, handling valueOf, even when unstable, is not going to be any worse perf-wise for the cases that are supposed to be fast.  So maybe we should change that.

> @@ -959,11 +1013,15 @@ class TypedArrayTemplate
>                      *dest++ = NativeType(JSVAL_TO_INT(v));
>                  } else if (JSVAL_IS_DOUBLE(v)) {
>                      *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
> +                } else if (JSVAL_IS_PRIMITIVE(v)) {
> +                    jsdouble dval = js_ValueToNumber(cx, &v);
> +                    *dest++ = NativeType(dval);
>                  } else {
> +                    if (ArrayTypeIsFloatingPoint()) {
> +                        *dest++ =
> NativeType(*JSVAL_TO_DOUBLE(cx->runtime->NaNValue));
> +                    } else {
> +                        *dest++ = NativeType(int32(0));
> +                    }
> 
> Inline method, macro, _something_ other than copy/paste, please!

Hm, yeah, I should be able to do a static inline helper for this.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Nope, the f2i function isn't LIR_f2i;

Ah, ok.  Good.

> I don't think we can specialize anything further at compile time.

You can avoid doing the nan-to-int conversion for objects, if we want to keep that behavior.  Not sure whether it matters.

> Not familiar with NumberTraits -- will look into it.

jsnum.h near the bottom.  Right now only int32 and jsdouble, but we can add more as needed.

> Well, NativeType(d) -> uint8_clamped(d)

Ah, I see.  OK.

> setIndex does take a NativeType, so maybe the cast/construction is redundant

Nah, I like the explicit cast/construction here.

> Well, like I said, concern over type stability

I'd like to understand that concern better before commenting more on the matter...  Is this a performance concern or a correctness concern?

Comment 30

[Brendan Eich [:brendan]]

> Is this a performance concern or a correctness concern?

This is a performance concern, which is the tail wagging the dog.

Vlad, what does the WebGL spec say? Can you link to chapter and verse?

/be

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Doesn't say anything right now -- we can write it either way.  If we can keep JS value-to-number conversion routines without major overhead, there's probably value in doing so.  The original implementation was very aggressive, using LIR_f2i etc., and going to the current code caused a good slowdown (1.5x-2x slowdown, can't remember exactly).  But it was still 10x faster than without typed arrays, so it's still a win; and I think we can get back much of that by emitting LIR for DoubleToECMA{Int32,Uint32} instead of making a function call.

Comment 32

[Andreas Gal :gal]

You can emit lir that does a compare of f2i == i2f and then jumps over the call if its an int. That should win you back most of it and be faithful to the spec.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: better patch, v3

Ok, updated patch with review comments.  This also fixes the JSVAL_HOLE crash, forgot to roll that up into here; this patch is also against tracemonkey, not m-c.

Takes review comments into account, except:

1) Didn't implement valueOf().  Let's do that in a followup patch.

2) Didn't do f2i(i2f()) optimization; followup patch again.

3) Couldn't quite figure out where NumberTraits could help; followup patch if necessary.

I'd like to get this in as-is because it fixes both a big crasher (the hole issue) as well as perf for common cases.

Comment 34

[Andreas Gal :gal]

Comment on attachment 432702 [details] [diff] [review]
better patch, v3

>+                d = js_ValueToNumber(cx, vp);

You can't just eat the error condition here. If JSVAL_IS_NULL(vp), then you must report error (return false). Yes, the API is fubar and the return value should be bool to make this clearer. I have whined about this in another bug somewhere. Also, the content of vp gets destroyed here (and replaced with a JSVAL_DOUBLE). Make sure that's ok. Warning 2: this code might trigger ->defaultValue() for objects, which we can't allow. So
make sure you don't hand in an object, or **** hits the fan (tracer can't see what's going on, stuff gets out of sync, bad times).

>+            } else if (*vp == JSVAL_VOID) {
>+                d = *JSVAL_TO_DOUBLE(cx->runtime->NaNValue);
>+            } else {
>+                d = (double) JSVAL_TO_BOOLEAN(*vp);
>+            }
>         } else {
>-            jsdouble d;
>-            if (JS_ValueToNumber(cx, *vp, &d))
>-                tarray->setIndex(index, NativeType(d));

Dito here. Don't eat the exception, and use js_ValueToNumber for consistency.

>-                if (JSVAL_IS_INT(v)) {
>-                    *dest++ = NativeType(JSVAL_TO_INT(v));
>-                } else if (JSVAL_IS_DOUBLE(v)) {
>-                    *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
>-                } else {
>-                    jsdouble dval;
>-                    if (!JS_ValueToNumber(cx, v, &dval))
>-                        return false;
>-                    *dest++ = NativeType(dval);
>-                }
>+                *dest++ = nativeFromValue(cx, v);

You used to hand JSVAL_HOLE into ValueToNumber here. I think that's fixed now. Please double check.

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #34)
> (From update of attachment 432702 [details] [diff] [review])
> >+                d = js_ValueToNumber(cx, vp);
> 
> You can't just eat the error condition here. If JSVAL_IS_NULL(vp), then you
> must report error (return false). Yes, the API is fubar and the return value
> should be bool to make this clearer. I have whined about this in another bug
> somewhere. Also, the content of vp gets destroyed here (and replaced with a
> JSVAL_DOUBLE). Make sure that's ok. Warning 2: this code might trigger
> ->defaultValue() for objects, which we can't allow. So
> make sure you don't hand in an object, or shit hits the fan (tracer can't see
> what's going on, stuff gets out of sync, bad times).

As per irc conversation, js_ValueToNumber can't ever fail with a string vp, and that's the only thing that we'll ever hand it here.  (vp does get overwritten, but with a boolean jsval, not a newly-allocated double.)  So I think this is fine here, though with a fixed ValueToNumber signature it'd be even cleaner.

> >+            } else if (*vp == JSVAL_VOID) {
> >+                d = *JSVAL_TO_DOUBLE(cx->runtime->NaNValue);
> >+            } else {
> >+                d = (double) JSVAL_TO_BOOLEAN(*vp);
> >+            }
> >         } else {
> >-            jsdouble d;
> >-            if (JS_ValueToNumber(cx, *vp, &d))
> >-                tarray->setIndex(index, NativeType(d));
> 
> Dito here. Don't eat the exception, and use js_ValueToNumber for consistency.

- code block, that's all removed.

> >-                if (JSVAL_IS_INT(v)) {
> >-                    *dest++ = NativeType(JSVAL_TO_INT(v));
> >-                } else if (JSVAL_IS_DOUBLE(v)) {
> >-                    *dest++ = NativeType(*JSVAL_TO_DOUBLE(v));
> >-                } else {
> >-                    jsdouble dval;
> >-                    if (!JS_ValueToNumber(cx, v, &dval))
> >-                        return false;
> >-                    *dest++ = NativeType(dval);
> >-                }
> >+                *dest++ = nativeFromValue(cx, v);
> 
> You used to hand JSVAL_HOLE into ValueToNumber here. I think that's fixed now.
> Please double check.

Yep, nativeFromValue explicitly checks for JSVAL_HOLE and doesn't call js_ValueToNumber.  It only calls it on JSVAL_IS_PRIMITIVE and v != JSVAL_HOLE, where it always succeeds.

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops -- missed a few cx->Runtime->NaNValue things.  changed all those to js_NaN.

Comment 37

[Brendan Eich [:brendan]]

(In reply to comment #35)
> As per irc conversation, js_ValueToNumber can't ever fail with a string vp, and
> that's the only thing that we'll ever hand it here.  (vp does get overwritten,
> but with a boolean jsval, not a newly-allocated double.)

Assert good *vp after.

> So I think this is
> fine here, though with a fixed ValueToNumber signature it'd be even cleaner.

That signature is confusing, but it won profiled cycles back. Is there an even better way that's fast and not confusing? File a new bug, cc: igor and me at least. Thanks,

/be

Comment 38

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, good call on the assert -- will add.

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/tracemonkey/rev/08f9be1e8315

Comment 40

[Boris Zbarsky [:bzbarsky]]

> 3) Couldn't quite figure out where NumberTraits could help; 

A followup bug is probably good, but the "Assign based on characteristics of the destination type" block could become a single:

  tarray->setIndex(NumberTraits<NativeType>::toSelfType(d));

call if we had NumberTraits for your various NativeType values, so the codepath selection happens at compile time, not runtime.  Then again, maybe the compiler optimizes this already, but it's not obvious.

As long as were there, we could replace js_ValueToNumber with StringToNumberType<double>, or if we really think the perf of the string case matters we could even template the StringToNumberType call on our actual NativeType and pass the result directly to setIndex in the string case.

That all said, I'm not sure why we do explicly js_DoubleToECMA(Ui|I)nt32 stuff in that "assign based ... " block but not in nativeFromValue.  Is there a reason for that difference?

Oh, and you can also use NumberTraits::NaN() in nativeFromValue to avoid that last branch on ArrayTypeIsFloatingPoint by replacing it with compile-time stuff.  Again, mostly code clarity if the compiler is good.

Don't forget to file the other followups too?

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, just filed:

bug 554458 - [typedarray] implement calling valueOf on object assignment

bug 554460 - [typedarray] add i2f(f2i(v)) == v check and use f2i value if true

bug 554461 - [typedarray] use more NumberTraits

> That all said, I'm not sure why we do explicly js_DoubleToECMA(Ui|I)nt32 stuff
> in that "assign based ... " block but not in nativeFromValue.  Is there a
> reason for that difference?

Oops.  bug 554464 - [typedarray] use correct DoubleTo{Uint,Int}32 conversion in nativeFromValue

Comment 42

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/08f9be1e8315