985733 - All setting of typed array elements from values should use To<integer type>() to compute the number to assign

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

Currently when we have

  var ta = new Uint8Array(16);
  ta[0] = v;

we assign, instead of |v| (which might not be a uint8_t in the above example), |convertToTargetType(v)|, where that method looks sort of like this:

  function convertToTargetType(v)
  {
    if (v is object)
    {
      if (target type is float)
        return NaN;
      return 0;
    }

    var n = ToNumber(v);
    return To<target type>(n);
  }

This was so that conversion wouldn't potentially GC and all, letting element-setting loops and such index into the same destination-array pointer throughout.  (Rather than having to recompute it every time because it might be invalidated.)  This optimization was always sort of dubious, given the many ways GC can be triggered, but it's what we implemented initially.

ES6 in standardizing typed arrays has decided these weird semantics aren't worth it -- people should just deal with ToNumber(v) potentially invoking valueOf, toString, and so on.  Other engines implement these not-so-unusual semantics, and have for a pretty long time.  (I've been meaning to file a bug to change us but haven't gotten around to it til now.)

This will involve somewhat cross-cutting changes -- to element-setting paths in JITs, maybe to TypedObject field-setting paths (not sure how similar/shared the code is), to definitions of typed array methods, &c.  It's not trivial, but not too tricky, either -- just requires getting a whole bunch of individual places changed correctly.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Refactor element-setting paths to make eventual conversion-semantics-switch simpler

No semantic change here, just cleaning up code.  Kinda sad this is -23 overall or so, means there was lots of pre-existing hoar.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Refactor element-setting paths to make eventual conversion-semantics-switch simpler

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Test for new semantics

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Switch to ToNumber+element-type-specific conversion semantics

Multiple JIT peoples here to be sure I didn't miss something else in need of change here.  :-)

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8395110 [details] [diff] [review]
Switch to ToNumber+element-type-specific conversion semantics

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

In MacroAssembler::convertValueToInt, we do:

    bool zeroObjects = behavior == IntConversion_ClampToUint8;
    ...
    if (zeroObjects)
        branchEqualTypeIfNeeded(MIRType_Object, maybeInput, tag, &isNull);

Objects are treated as 0 if we're clamping. It seems you'd want a testcase with an Uint8ClampedArray SETELEM where the value has different types to exercise this code.

Comment 6

[Jan de Mooij [:jandem]]

(Good we're fixing this btw, it always seemed like a weird quirk. Easy to implement though ;))

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8395110 [details] [diff] [review]
Switch to ToNumber+element-type-specific conversion semantics

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

Looks okay to me, with the confusing comment + clamping tests addressed.

As a note, the Ion TypedArray SETELEM doesn't need any changes because we don't generate stubs for strings and objects anyways.

::: js/src/vm/TypedArrayObject.cpp
@@ +915,5 @@
> +            len = Min(len, thisTypedArray->length());
> +            if (i >= len)
> +                break;
> +
> +            // Compute every iteration in case getElement acts wacky.

Is this comment meant for the thisTypedArray->length() change above? And why is that needed, because of possible setters that modify .length?

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Test, updated

With a test for the issue jandem pointed out.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Patch, updated

Any other random places to change?

(Incidentally, those extra bits of code to change are crazy weird.  I'm going to refactor them to read better/more sensibly, but definitely not in this bug.)

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8396740 [details] [diff] [review]
Patch, updated

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

r=me on the jit/ changes.

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8395108 [details] [diff] [review]
Refactor element-setting paths to make eventual conversion-semantics-switch simpler

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

::: js/src/jsobj.cpp
@@ +5028,5 @@
> +            if (mode == ParallelExecution) {
> +                // Bail if converting the value might invoke user-defined
> +                // conversions.
> +                if (vp.isObject())
> +                    return false;

Kind of harsh to assume any object is dangerous, but that can always be weakened later.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8396737 [details] [diff] [review]
Test, updated

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

Nice fuzzer food too.

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8396740 [details] [diff] [review]
Patch, updated

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

This reads much better.

::: js/src/vm/TypedArrayObject.cpp
@@ +863,5 @@
>          }
>  
> +        double d;
> +        MOZ_ASSERT(v.isString() || v.isObject());
> +        if (v.isString() ? !StringToNumber(cx, v.toString(), &d) : !ToNumber(cx, v, &d))

We may need a cage match to decide this critical issue, but I somewhat prefer

  if (!(v.isString() ? StringToNumber(cx, v.toString(), &d) : ToNumber(cx, v, &d)))

@@ +872,5 @@
>      }
>  
>      static bool
>      copyFromArray(JSContext *cx, HandleObject thisTypedArrayObj,
> +                  HandleObject source, uint32_t len, uint32_t offset = 0)

Thank you thank you for renaming that stupid 'ar' parameter.

@@ +887,5 @@
> +            // the first potentially side-effectful lookup or conversion.
> +            uint32_t bound = Min(source->getDenseInitializedLength(), len);
> +
> +            NativeType *dest = &static_cast<NativeType*>(thisTypedArray->viewData())[offset];
> +            SkipRoot skipDest(cx, &dest);

This will need a rebase; SkipRoot is gone.

Comment 14

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9585eb8a3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa4e0b93705
https://hg.mozilla.org/integration/mozilla-inbound/rev/58ccbf57d72a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb9585eb8a3a
https://hg.mozilla.org/mozilla-central/rev/3fa4e0b93705
https://hg.mozilla.org/mozilla-central/rev/58ccbf57d72a