Closed Bug 985733 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Waldo, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
No semantic change here, just cleaning up code.  Kinda sad this is -23 overall or so, means there was lots of pre-existing hoar.
Attachment #8394420 - Flags: review?(sphink)
Attachment #8394420 - Flags: review?(sphink)
Attached patch Test for new semantics (obsolete) — Splinter Review
Attachment #8395109 - Flags: review?(sphink)
Multiple JIT peoples here to be sure I didn't miss something else in need of change here.  :-)
Attachment #8395110 - Flags: review?(sphink)
Attachment #8395110 - Flags: review?(jdemooij)
Attachment #8395110 - Flags: review?(shu)
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.
Attachment #8395110 - Flags: review?(jdemooij)
(Good we're fixing this btw, it always seemed like a weird quirk. Easy to implement though ;))
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?
Attachment #8395110 - Flags: review?(shu) → review+
Attached patch Test, updatedSplinter Review
With a test for the issue jandem pointed out.
Attachment #8395109 - Attachment is obsolete: true
Attachment #8395109 - Flags: review?(sphink)
Attachment #8396737 - Flags: review?(sphink)
Attached patch Patch, updatedSplinter Review
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.)
Attachment #8395110 - Attachment is obsolete: true
Attachment #8395110 - Flags: review?(sphink)
Attachment #8396740 - Flags: review?(sphink)
Attachment #8396740 - Flags: review?(jdemooij)
Comment on attachment 8396740 [details] [diff] [review]
Patch, updated

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

r=me on the jit/ changes.
Attachment #8396740 - Flags: review?(jdemooij) → review+
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.
Attachment #8395108 - Flags: review?(sphink) → review+
Comment on attachment 8396737 [details] [diff] [review]
Test, updated

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

Nice fuzzer food too.
Attachment #8396737 - Flags: review?(sphink) → review+
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.
Attachment #8396740 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/cb9585eb8a3a
https://hg.mozilla.org/mozilla-central/rev/3fa4e0b93705
https://hg.mozilla.org/mozilla-central/rev/58ccbf57d72a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1001547
Duplicate of this bug: 914908
Duplicate of this bug: 554458
You need to log in before you can comment on or make changes to this bug.