Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090353003:
Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. (Closed)
DescriptionAmend DataView, ArrayBuffer, and TypedArray methods to use ToIndex.
The spec was modified to relax some requirements which implementors had not been
enforcing. Part of this process involved introducing a new abstract operation
ToIndex, which had partial overlap with our existing semantics as well as some
differences (most notably treating undefined as 0). Test262 tests were introduced to
check for the new semantics, some of which we were failing. This patch amends the
parts of our implementation corresponding to specification algorithms which use
ToIndex to follow its semantics precisely.
BUG=v8:4784, v8:5120
Committed: https://crrev.com/09720349ea058d178521ec58d0a5676443a5a132
Cr-Commit-Position: refs/heads/master@{#37406}
Patch Set 1 #Patch Set 2 : enable test #
Total comments: 7
Patch Set 3 : clean up tests #Patch Set 4 : rebase #
Total comments: 5
Patch Set 5 : better error message #Patch Set 6 : stricter test #
Total comments: 5
Patch Set 7 : comment #Patch Set 8 : comment #
Messages
Total messages: 44 (17 generated)
bakkot@google.com changed reviewers: + littledan@chromium.org
Description was changed from ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. BUG=v8:4784,v8:5120 ========== to ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. The spec was modified to relax some requirements which implementors had not been enforcing. Part of this process involved introducing a new abstract operation ToIndex, which had partial overlap with our existing semantics as well as some differences (most notably treating undefined as 0). Test262 tests were introduced to check for the new semantics, some of which we were failing. This patch amends the parts of our implementation corresponding to specification algorithms which use ToIndex to follow its semantics precisely. BUG=v8:4784,v8:5120 ==========
https://codereview.chromium.org/2090353003/diff/20001/src/runtime/runtime-obj... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2090353003/diff/20001/src/runtime/runtime-obj... src/runtime/runtime-object.cc:893: } I don't see anyone who calls %ToIndex. Why was the runtime function added? https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/es6/typeda... File test/mjsunit/es6/typedarray.js (right): https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/es6/typeda... test/mjsunit/es6/typedarray.js:49: }, RangeError); Eh, can you run this with msan or something before submitting? It might need to be marked as off for that mode. https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/harmony/da... File test/mjsunit/harmony/dataview-accessors.js (right): https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/harmony/da... test/mjsunit/harmony/dataview-accessors.js:454: a.setFloat64(1); Can you assert that these get values that make sense for 0 being passed as the argument?
adamk@chromium.org changed reviewers: + adamk@chromium.org
https://codereview.chromium.org/2090353003/diff/20001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2090353003/diff/20001/src/js/typedarray.js#ne... src/js/typedarray.js:152: if (length > %_MaxSmi()) { // note: this is not per spec; we just don't want to allocate excessively large arrays Drive-by comment, here and elsewhere: please limit lines to 80 characters. (we have a lint check that catches this for c++ code, not sure why we don't have one for JS).
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2090353003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/4054)
Rebase and try again?
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Rebased; ptal. Not sure what it means that v8_linux64_sanitizer_coverage_rel failed. https://codereview.chromium.org/2090353003/diff/20001/src/runtime/runtime-obj... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2090353003/diff/20001/src/runtime/runtime-obj... src/runtime/runtime-object.cc:893: } On 2016/06/23 21:45:32, Dan Ehrenberg wrote: > I don't see anyone who calls %ToIndex. Why was the runtime function added? Removed. https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/es6/typeda... File test/mjsunit/es6/typedarray.js (right): https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/es6/typeda... test/mjsunit/es6/typedarray.js:49: }, RangeError); On 2016/06/23 21:45:32, Dan Ehrenberg wrote: > Eh, can you run this with msan or something before submitting? It might need to > be marked as off for that mode. Yes indeed; fails with "MemorySanitizer failed to allocate 0xffffffffffff bytes". Left this as it was. It shouldn't even be trying to allocate that memory, though (the test is that this attempt fails), so I'm not entirely sure what's going on. https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/harmony/da... File test/mjsunit/harmony/dataview-accessors.js (right): https://codereview.chromium.org/2090353003/diff/20001/test/mjsunit/harmony/da... test/mjsunit/harmony/dataview-accessors.js:454: a.setFloat64(1); On 2016/06/23 21:45:32, Dan Ehrenberg wrote: > Can you assert that these get values that make sense for 0 being passed as the > argument? Done.
Description was changed from ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. The spec was modified to relax some requirements which implementors had not been enforcing. Part of this process involved introducing a new abstract operation ToIndex, which had partial overlap with our existing semantics as well as some differences (most notably treating undefined as 0). Test262 tests were introduced to check for the new semantics, some of which we were failing. This patch amends the parts of our implementation corresponding to specification algorithms which use ToIndex to follow its semantics precisely. BUG=v8:4784,v8:5120 ========== to ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. The spec was modified to relax some requirements which implementors had not been enforcing. Part of this process involved introducing a new abstract operation ToIndex, which had partial overlap with our existing semantics as well as some differences (most notably treating undefined as 0). Test262 tests were introduced to check for the new semantics, some of which we were failing. This patch amends the parts of our implementation corresponding to specification algorithms which use ToIndex to follow its semantics precisely. BUG=v8:4784,v8:5120 ==========
https://codereview.chromium.org/2090353003/diff/60001/src/messages.h File src/messages.h (right): https://codereview.chromium.org/2090353003/diff/60001/src/messages.h#newcode341 src/messages.h:341: T(InvalidIndex, "Invalid index") \ The error message here could be better. Maybe Object::ToIndex should take an error index as a parameter like the runtime version. For bonus points, you could make these error messages include the offending index (with %). https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... File test/mjsunit/harmony/dataview-accessors.js (right): https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... test/mjsunit/harmony/dataview-accessors.js:433: a["set" + type](); Not sure why you're calling this function and throwing away the value. The return value is specified, so better to check that you're getting what you expect.
It's safe to ignore v8_linux64_sanitizer_coverage_rel IMO; it seems to fail for a lot of patches.
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed comments. https://codereview.chromium.org/2090353003/diff/60001/src/messages.h File src/messages.h (right): https://codereview.chromium.org/2090353003/diff/60001/src/messages.h#newcode341 src/messages.h:341: T(InvalidIndex, "Invalid index") \ On 2016/06/28 at 23:50:14, Dan Ehrenberg wrote: > The error message here could be better. Maybe Object::ToIndex should take an error index as a parameter like the runtime version. For bonus points, you could make these error messages include the offending index (with %). Done. https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... File test/mjsunit/harmony/dataview-accessors.js (right): https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... test/mjsunit/harmony/dataview-accessors.js:433: a["set" + type](); On 2016/06/28 at 23:50:14, Dan Ehrenberg wrote: > Not sure why you're calling this function and throwing away the value. The return value is specified, so better to check that you're getting what you expect. Done, but I'm not sure how valuable it is to assert that it returns undefined. We're calling these lines for their side effects, which we're verifying with the next line.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... File test/mjsunit/harmony/dataview-accessors.js (right): https://codereview.chromium.org/2090353003/diff/60001/test/mjsunit/harmony/da... test/mjsunit/harmony/dataview-accessors.js:433: a["set" + type](); On 2016/06/29 at 19:01:45, bakkot wrote: > On 2016/06/28 at 23:50:14, Dan Ehrenberg wrote: > > Not sure why you're calling this function and throwing away the value. The return value is specified, so better to check that you're getting what you expect. > > Done, but I'm not sure how valuable it is to assert that it returns undefined. We're calling these lines for their side effects, which we're verifying with the next line. It's important that each function return the right value because whatever we implement may be depended on by users. For example, another way to design DataViews would be to return this, allowing chaining. The spec says not to do this, though. https://codereview.chromium.org/2090353003/diff/100001/src/js/runtime.js File src/js/runtime.js (right): https://codereview.chromium.org/2090353003/diff/100001/src/js/runtime.js#newc... src/js/runtime.js:50: if (i < 0 || i > kMaxSafeInteger) throw MakeRangeError(rangeErrorIndex); Pass x as an argument here? https://codereview.chromium.org/2090353003/diff/100001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2090353003/diff/100001/src/js/typedarray.js#n... src/js/typedarray.js:153: if (length > %_MaxSmi()) { // note: this is not per spec; we just don't want to allocate excessively large arrays Line length, also see note below https://codereview.chromium.org/2090353003/diff/100001/src/js/typedarray.js#n... src/js/typedarray.js:194: if (length > %_MaxSmi()) { // note: this is not per spec; we just don't want to allocate excessively large arrays Line length. Also, "we don't want to" is "we are unable to in our current representation" (which uses smis)
https://codereview.chromium.org/2090353003/diff/100001/src/js/runtime.js File src/js/runtime.js (right): https://codereview.chromium.org/2090353003/diff/100001/src/js/runtime.js#newc... src/js/runtime.js:50: if (i < 0 || i > kMaxSafeInteger) throw MakeRangeError(rangeErrorIndex); On 2016/06/29 at 19:13:43, Dan Ehrenberg wrote: > Pass x as an argument here? The messages passed to this function don't take an argument. https://codereview.chromium.org/2090353003/diff/100001/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2090353003/diff/100001/src/js/typedarray.js#n... src/js/typedarray.js:194: if (length > %_MaxSmi()) { // note: this is not per spec; we just don't want to allocate excessively large arrays On 2016/06/29 at 19:13:43, Dan Ehrenberg wrote: > Line length. > > Also, "we don't want to" is "we are unable to in our current representation" (which uses smis) Addressed.
lgtm
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bakkot@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. The spec was modified to relax some requirements which implementors had not been enforcing. Part of this process involved introducing a new abstract operation ToIndex, which had partial overlap with our existing semantics as well as some differences (most notably treating undefined as 0). Test262 tests were introduced to check for the new semantics, some of which we were failing. This patch amends the parts of our implementation corresponding to specification algorithms which use ToIndex to follow its semantics precisely. BUG=v8:4784,v8:5120 ========== to ========== Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex. The spec was modified to relax some requirements which implementors had not been enforcing. Part of this process involved introducing a new abstract operation ToIndex, which had partial overlap with our existing semantics as well as some differences (most notably treating undefined as 0). Test262 tests were introduced to check for the new semantics, some of which we were failing. This patch amends the parts of our implementation corresponding to specification algorithms which use ToIndex to follow its semantics precisely. BUG=v8:4784,v8:5120 Committed: https://crrev.com/09720349ea058d178521ec58d0a5676443a5a132 Cr-Commit-Position: refs/heads/master@{#37406} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/09720349ea058d178521ec58d0a5676443a5a132 Cr-Commit-Position: refs/heads/master@{#37406}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2113593002/ by hablich@chromium.org. The reason for reverting is: Speculative revert to unblock roll: https://codereview.chromium.org/2107223003/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
