Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609913002:
[builtins] throw if TypedArray buffer is detached during iteration (Closed)
Description[builtins] throw if TypedArray buffer is detached during iteration
Per spec change in https://github.com/tc39/ecma262/pull/724, this adds
the exception thrown when a TypedArray's array buffer is detached at
some point during iteration, after the iterator has already been
created.
BUG=v8:5388
R=littledan@chromium.org, bmeurer@chromium.org, petermarshall@chromium.org
Review-Url: https://codereview.chromium.org/2609913002
Cr-Commit-Position: refs/heads/master@{#42048}
Committed: https://chromium.googlesource.com/v8/v8/+/5c6e79e184f8e1f14802cc7664f58652651dcc2a
Patch Set 1 #
Total comments: 1
Patch Set 2 : maybe fix the test #
Total comments: 4
Patch Set 3 : remove optimization state test and fix heap constant #
Total comments: 1
Patch Set 4 : fix compiler error #Patch Set 5 : fix the other compiler error #
Messages
Total messages: 34 (23 generated)
The CQ bit was checked by caitp@igalia.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...
please take a look when time permits, thanks!
Code seems fine AFAICT. I couldn't say no to an implementation of spec text I wrote... but... This is part of a bigger issue, tracked in https://bugs.chromium.org/p/v8/issues/detail?id=4895 and https://github.com/tc39/ecma262/issues/678 . It could be tracked better--namely, I don't know how web-compatible it would be to throw in all of the various cases that the spec says to throw. I bet this is web-compatible, but I think it would be worth it to think about this as part of a bigger project to see if we can ship the throwing semantics in a more holistic way. Anyway, I think it'd be OK for us to ship these semantics now; I'll just leave the review of the builtins code for someone who's been more involved in this part of them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14944) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
https://codereview.chromium.org/2609913002/diff/1/test/mjsunit/es6/array-iter... File test/mjsunit/es6/array-iterator-detached.js (right): https://codereview.chromium.org/2609913002/diff/1/test/mjsunit/es6/array-iter... test/mjsunit/es6/array-iterator-detached.js:41: "Turbo should be optimized using TurboFan backend."); these sorts of assertions are hard to get right :(
The CQ bit was checked by caitp@igalia.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.
https://codereview.chromium.org/2609913002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2609913002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2281: assembler.HeapConstant(assembler.factory()->NewStringFromAsciiChecked( The CodeStubAssembler generates a schedule immediately, which means that in the worst case, it'll block one register from this point on to keep the constant. Currently this shouldn't happen, but there's no guarantee. I'd suggest to just leave the HeapConstant at the runtime call uses below. https://codereview.chromium.org/2609913002/diff/20001/test/mjsunit/es6/array-... File test/mjsunit/es6/array-iterator-detached.js (right): https://codereview.chromium.org/2609913002/diff/20001/test/mjsunit/es6/array-... test/mjsunit/es6/array-iterator-detached.js:54: assertTrue(state !== kInterpreted && state !== kNo, This is very brittle. I'd rather have a high-level blackbox test, which doesn't require peaking into the optimizatio state.
The CQ bit was checked by caitp@igalia.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...
https://codereview.chromium.org/2609913002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2609913002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2281: assembler.HeapConstant(assembler.factory()->NewStringFromAsciiChecked( On 2017/01/03 18:09:39, Benedikt Meurer wrote: > The CodeStubAssembler generates a schedule immediately, which means that in the > worst case, it'll block one register from this point on to keep the constant. > Currently this shouldn't happen, but there's no guarantee. I'd suggest to just > leave the HeapConstant at the runtime call uses below. Good point, done https://codereview.chromium.org/2609913002/diff/20001/test/mjsunit/es6/array-... File test/mjsunit/es6/array-iterator-detached.js (right): https://codereview.chromium.org/2609913002/diff/20001/test/mjsunit/es6/array-... test/mjsunit/es6/array-iterator-detached.js:54: assertTrue(state !== kInterpreted && state !== kNo, On 2017/01/03 18:09:39, Benedikt Meurer wrote: > This is very brittle. I'd rather have a high-level blackbox test, which doesn't > require peaking into the optimizatio state. Fair enough
https://codereview.chromium.org/2609913002/diff/40001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2609913002/diff/40001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2281: Node* operation = assembler.factory()->NewStringFromAsciiChecked( bleh, editing this in vim caused some problems here :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) 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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18510) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14955)
The CQ bit was checked by caitp@igalia.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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18541) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18603) 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/14956)
The CQ bit was checked by caitp@igalia.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.
Thanks! LGTM.
On 2017/01/03 19:56:44, Benedikt Meurer wrote: > Thanks! LGTM. Thanks --- Dan, I assume this works for you? I wasn't sure if you were asking for a use counter or something.
lgtm Works for me
The CQ bit was checked by caitp@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483474180669630,
"parent_rev": "db7f0169f5cca3beb64f7460527bbd508a0fb94a", "commit_rev":
"5c6e79e184f8e1f14802cc7664f58652651dcc2a"}
Message was sent while issue was closed.
Description was changed from ========== [builtins] throw if TypedArray buffer is detached during iteration Per spec change in https://github.com/tc39/ecma262/pull/724, this adds the exception thrown when a TypedArray's array buffer is detached at some point during iteration, after the iterator has already been created. BUG=v8:5388 R=littledan@chromium.org, bmeurer@chromium.org, petermarshall@chromium.org ========== to ========== [builtins] throw if TypedArray buffer is detached during iteration Per spec change in https://github.com/tc39/ecma262/pull/724, this adds the exception thrown when a TypedArray's array buffer is detached at some point during iteration, after the iterator has already been created. BUG=v8:5388 R=littledan@chromium.org, bmeurer@chromium.org, petermarshall@chromium.org Review-Url: https://codereview.chromium.org/2609913002 Cr-Commit-Position: refs/heads/master@{#42048} Committed: https://chromium.googlesource.com/v8/v8/+/5c6e79e184f8e1f14802cc7664f58652651... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/5c6e79e184f8e1f14802cc7664f58652651... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
