Bug 83818 - Incorrect TypedArray#set behavior
Summary: Incorrect TypedArray#set behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-12 13:50 PDT by Stéphan Kochen
Modified: 2013-08-23 13:41 PDT (History)
11 users (show)

See Also:

Attachments
Test case (458 bytes, application/x-javascript)
2012-04-12 13:50 PDT, Stéphan Kochen 		no flags Details
the patch (7.20 KB, patch)
2013-08-22 22:58 PDT, Filip Pizlo 		oliver: review+
Details | Formatted Diff | Diff
test for the case where we optimize with memmove (459 bytes, application/x-javascript)
2013-08-22 22:59 PDT, Filip Pizlo 		no flags Details
test for overlapping elements of the same size (482 bytes, application/x-javascript)
2013-08-22 23:00 PDT, Filip Pizlo 		no flags Details
test where the source elements are smaller than the destination elements (2.65 KB, application/x-javascript)
2013-08-22 23:00 PDT, Filip Pizlo 		no flags Details
test where the destination elements are smaller than the source elements (1.56 KB, application/x-javascript)
2013-08-22 23:01 PDT, Filip Pizlo 		no flags Details
the patch (28.14 KB, patch)
2013-08-23 13:17 PDT, Filip Pizlo 		oliver: review+
Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Stéphan Kochen 2012-04-12 13:50:14 PDT 
Created attachment 136964 [details]
Test case

Copying between two TypedArrays of different types, but backed by the same ArrayBuffer, using TypedArray#set can lead to incorrect results.

The attached test case can be run straight in the Developer Tools console. It contains three asserts of which the third fails:

    Uint16 value: 1285 == 1285 ? true
    First Uint16 element: 5 == 5 ? true
    Second Uint16 element: 0 == 5 ? false

I'm guessing a[0] is written before b[1] is read. According to the spec, the copy should behave as if a temporary buffer is inbetween.

Tested on a nightly, that describes itself as: Version 5.1.5 (7534.55.3, r113983)

Thread on WebGL mailing list: https://www.khronos.org/webgl/public-mailing-list/archives/1204/msg00172.html
Comment 1 Oliver Hunt 2012-04-12 14:10:55 PDT 
Yay for a spec that claims performance is important, but actively sabotages optimisations.
Comment 2 Kenneth Russell 2012-04-17 17:52:51 PDT 
As discussed on the public_webgl list, the original intent of the typed array spec was that this would throw TypeError. set() taking a typed array was supposed to be implementable using memmove().

Unfortunately, it turns out that this overload was illegal according to Web IDL. Recent Web IDL spec changes make the overload legal, but require that the version taking a JS array be called for all typed array view types other than the receiver's.
Comment 3 Filip Pizlo 2013-08-22 19:17:51 PDT 
I'll look at it.
Comment 4 Filip Pizlo 2013-08-22 22:58:28 PDT 
Created attachment 209432 [details]
the patch

Note, I have a bunch of tests for this - I'll upload the ad-hoc tests I wrote and I will LayoutTest-ify them later.  I plan to also LayoutTest-ify Stéphan's original test case.
Comment 5 Filip Pizlo 2013-08-22 22:59:39 PDT 
Created attachment 209433 [details]
test for the case where we optimize with memmove
Comment 6 Filip Pizlo 2013-08-22 23:00:07 PDT 
Created attachment 209434 [details]
test for overlapping elements of the same size
Comment 7 Filip Pizlo 2013-08-22 23:00:36 PDT 
Created attachment 209435 [details]
test where the source elements are smaller than the destination elements
Comment 8 Filip Pizlo 2013-08-22 23:01:01 PDT 
Created attachment 209436 [details]
test where the destination elements are smaller than the source elements
Comment 9 Filip Pizlo 2013-08-23 13:17:57 PDT 
Created attachment 209501 [details]
the patch

The patch including tests and some minor changes.
Comment 10 Mark Hahnenberg 2013-08-23 13:35:26 PDT 
Comment on attachment 209501 [details]
the patch

r=me too
Comment 11 Filip Pizlo 2013-08-23 13:41:21 PDT 
Landed in http://trac.webkit.org/changeset/154518