Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368323002:
[builtins] adapt arguments for Builtins::kIteratorPrototypeIterator (Closed)
Description[builtins] adapt arguments for Builtins::kIteratorPrototypeIterator
BUG=chromium:650172
R=mstarzinger@chromium.org, bmeurer@chromium.org
Committed: https://crrev.com/8fea775784d340d899bdab6a0248bf5d4daf3f07
Cr-Commit-Position: refs/heads/master@{#39760}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Remove test flag #
Messages
Total messages: 14 (3 generated)
PTAL --- I think this test actually crashes release builds without --ignition, too, but I wasn't sure if ignition would run without the flag.
Fix is good, test not yet. https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition Does it also repro with --ignition-staging? If so, then you don't need the Flags: here. https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:7: var iterator = [].entries().__proto__.__proto__[Symbol.iterator]; How about using Array.prototype[Symbol.iterator].__proto__ instead? that should be the same. https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:8: print(1/iterator(-1E-300)); I guess you don't need the print or the division, please remove that.
https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition On 2016/09/26 18:30:31, Benedikt Meurer wrote: > Does it also repro with --ignition-staging? If so, then you don't need the > Flags: here. It does if the division is there to cause the x87 PF (might not crash on other architectures though). Ignition crashes in more more cross-platform ways, including CHECK failures when tracing is enabled when this kind of bug occurs. But alright, I can live with that. sgtm https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:7: var iterator = [].entries().__proto__.__proto__[Symbol.iterator]; On 2016/09/26 18:30:31, Benedikt Meurer wrote: > How about using Array.prototype[Symbol.iterator].__proto__ instead? that should > be the same. SGTM https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:8: print(1/iterator(-1E-300)); On 2016/09/26 18:30:31, Benedikt Meurer wrote: > I guess you don't need the print or the division, please remove that. well, the division causes an x87 PF (because not enough values on the ST regs, because iterator() doesn't clean up its stack, because no arguments adaptor frame) --- so the division makes the bug clear (and makes it easier to crash). The print() is mostly for debugging the test, but it doesn't really impact built bots or manusal testing, so I think it's not too bad. I'll remove it it if you want though)
https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:7: var iterator = [].entries().__proto__.__proto__[Symbol.iterator]; On 2016/09/26 21:51:47, caitp wrote: > On 2016/09/26 18:30:31, Benedikt Meurer wrote: > > How about using Array.prototype[Symbol.iterator].__proto__ instead? that > should > > be the same. > > SGTM Actually, doing that doesn't hit the crash. [].entries() -> instantiates ArrayIterator ArrayIterator.__proto__ -> instance of %ArrayIteratorPrototype% %IteratorPrototype%.__proto__-> instance of %IteratorPrototype% vs Array -> %Array% Array.prototype -> %ArrayPrototype% Array.prototype[Symbol.iterator] -> %ArrayProto_values% %ArrayProto_values%.__proto__ -> %FunctionPrototype% %ArrayProto_values%.prototype -> undefined So, you need to get at %IteratorPrototype% via an instance of the iterator.
Ok, thanks for clarifying. LGTM (modulo the Flags:). https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition Please nuke the Flags: if it also crashes with --ignition-staging. The reason behind this is that we want to nuke the --ignition flag in favour of --ignition-staging (plus --turbo). https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:7: var iterator = [].entries().__proto__.__proto__[Symbol.iterator]; On 2016/09/26 22:12:52, caitp wrote: > On 2016/09/26 21:51:47, caitp wrote: > > On 2016/09/26 18:30:31, Benedikt Meurer wrote: > > > How about using Array.prototype[Symbol.iterator].__proto__ instead? that > > should > > > be the same. > > > > SGTM > > Actually, doing that doesn't hit the crash. > > [].entries() -> instantiates ArrayIterator > ArrayIterator.__proto__ -> instance of %ArrayIteratorPrototype% > %IteratorPrototype%.__proto__-> instance of %IteratorPrototype% > > vs > > Array -> %Array% > Array.prototype -> %ArrayPrototype% > Array.prototype[Symbol.iterator] -> %ArrayProto_values% > %ArrayProto_values%.__proto__ -> %FunctionPrototype% > %ArrayProto_values%.prototype -> undefined > > So, you need to get at %IteratorPrototype% via an instance of the iterator. Acknowledged. https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:8: print(1/iterator(-1E-300)); On 2016/09/26 21:51:47, caitp wrote: > On 2016/09/26 18:30:31, Benedikt Meurer wrote: > > I guess you don't need the print or the division, please remove that. > > well, the division causes an x87 PF (because not enough values on the ST regs, > because iterator() doesn't clean up its stack, because no arguments adaptor > frame) --- so the division makes the bug clear (and makes it easier to crash). > > The print() is mostly for debugging the test, but it doesn't really impact built > bots or manusal testing, so I think it's not too bad. I'll remove it it if you > want though) Acknowledged.
https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition On 2016/09/27 03:33:24, Benedikt Meurer wrote: > Please nuke the Flags: if it also crashes with --ignition-staging. The reason > behind this is that we want to nuke the --ignition flag in favour of > --ignition-staging (plus --turbo). So, you're saying you'd prefer `Flags: --ignition-staging --turbo`? Or, just no flags? "nuke the Flags:" is telling me the latter, and "nuke --ignition in favour of --ignition-staging" is telling me the former.
https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition Sorry for the confusion: We run --ignition-staging as part of the bot defaults, which means you don't need to include it here explicitly in Flags:. So if it crashes with --ignition-staging, then remove the Flags: line. If it only crashes with --ignition, then keep the Flags: line.
https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... File test/mjsunit/es6/regress/regress-650172.js (right): https://codereview.chromium.org/2368323002/diff/1/test/mjsunit/es6/regress/re... test/mjsunit/es6/regress/regress-650172.js:5: // Flags: --ignition On 2016/09/27 09:14:29, Benedikt Meurer wrote: > Sorry for the confusion: We run --ignition-staging as part of the bot defaults, > which means you don't need to include it here explicitly in Flags:. So if it > crashes with --ignition-staging, then remove the Flags: line. If it only crashes > with --ignition, then keep the Flags: line. I see, done.
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2368323002/#ps10001 (title: "Remove test flag")
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 #2 (id:10001)
Message was sent while issue was closed.
Description was changed from ========== [builtins] adapt arguments for Builtins::kIteratorPrototypeIterator BUG=chromium:650172 R=mstarzinger@chromium.org, bmeurer@chromium.org ========== to ========== [builtins] adapt arguments for Builtins::kIteratorPrototypeIterator BUG=chromium:650172 R=mstarzinger@chromium.org, bmeurer@chromium.org Committed: https://crrev.com/8fea775784d340d899bdab6a0248bf5d4daf3f07 Cr-Commit-Position: refs/heads/master@{#39760} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8fea775784d340d899bdab6a0248bf5d4daf3f07 Cr-Commit-Position: refs/heads/master@{#39760} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
