Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings.
The bug is in the common C++ code, specifically IntegralTypedArrayBase.h. It's explicitly clamping the incoming double value to the element type's minimum and maximum values, where it should be using casting behavior.
Also the conformance test array-unit-tests.html is failing due to 0.6 is mapped to 1 instead of 0. According to Web IDL type mapping, floor(abs(x)) * sign(x) is the correct behavior. Maybe it could be fixed in one patch as the code is in the same function.
The rounding mode failure in array-unit-tests.html in Chromium is a different problem. That fix has been committed to V8 and we are waiting for it to be rolled into Chromium.
(In reply to comment #0) > Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings. Looking at the typed array spec, it could be more clear about these conversions. There is one statement in the "omittable setter" part of section 5 that gives a link to the WebIDL spec. But I think it would be better call this out into its own section. I've opened a spec bug about this: https://www.khronos.org/bugzilla/show_bug.cgi?id=420. It's in the WebGL area, which I suppose is an appropriate place for it for now.
(In reply to comment #4) > (In reply to comment #0) > > Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings. > > Looking at the typed array spec, it could be more clear about these conversions. There is one statement in the "omittable setter" part of section 5 that gives a link to the WebIDL spec. But I think it would be better call this out into its own section. > > I've opened a spec bug about this: https://www.khronos.org/bugzilla/show_bug.cgi?id=420. It's in the WebGL area, which I suppose is an appropriate place for it for now. For the record, the spec bug has been fixed; the typed array spec is much more clear about rounding mode and type conversion behavior.
Created attachment 82729 [details] Patch
Comment on attachment 82729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82729&action=review Generally fine, one nit, and one question about the necessity of LargestIntType in the template. r=me, after settling those. > Source/WebCore/ChangeLog:13 > + required template parameter. I don't understand the second parameter to IntegralTypedArrayBase. It's always a 64 bit int, which it would have to be to hold a any possible double. I don't think having it be a uint64 in some places is useful because a signed 64 bit number is plenty to hold all possible double values. I think JS requires numbers to be doubles so why not just eliminate it? Seems like unnecessary future-proofing > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:137 > + unsigned length = srcArray->get(exec, JSC::Identifier(exec, "length")).toUInt32(exec); It would be more consistent to use uint32_t instead of unsigned here?
(In reply to comment #7) > (From update of attachment 82729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82729&action=review > > Generally fine, one nit, and one question about the necessity of LargestIntType in the template. r=me, after settling those. > > > Source/WebCore/ChangeLog:13 > > + required template parameter. > > I don't understand the second parameter to IntegralTypedArrayBase. It's always a 64 bit int, which it would have to be to hold a any possible double. I don't think having it be a uint64 in some places is useful because a signed 64 bit number is plenty to hold all possible double values. I think JS requires numbers to be doubles so why not just eliminate it? Seems like unnecessary future-proofing I didn't experiment enough, but you're right; the intermediate cast can always be to int64_t, so the LargestIntType template parameter can be eliminated. This simplifies the patch. > > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:137 > > + unsigned length = srcArray->get(exec, JSC::Identifier(exec, "length")).toUInt32(exec); > > It would be more consistent to use uint32_t instead of unsigned here? OK, I'll make this change.
Committed r78919: <http://trac.webkit.org/changeset/78919>