|
|
Created:
4 years, 7 months ago by Dan Ehrenberg Modified:
4 years, 6 months ago Reviewers:
caitp (gmail), adamk, Yang CC:
oth, rmcilroy, v8-reviews_googlegroups.com, Yang Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[esnext] Fix various callsites to use is_resumable, not is_generator
Async functions are built out of generators, but the
SharedFunctionInfo returns false for is_generator. is_resumable is
the broader query. This patch fixes many parts of V8 to refer
to is_resumable as appropriate.
One incidental change is to remove a check for generators extending
classes. This is part of a general check for constructors being the
only thing which can extend classes, so it is removed here and the
error message for the general case is made more accurate.
BUG=v8:4483
Committed: https://crrev.com/46253e743df20c67bd524cf6f4288f549cb72739
Cr-Commit-Position: refs/heads/master@{#36621}
Patch Set 1 #
Total comments: 10
Patch Set 2 : formatting #Patch Set 3 : Fix test expectations #Patch Set 4 : fix test better #Patch Set 5 : Add CHECK #Patch Set 6 : fix dcheck #Patch Set 7 : #Patch Set 8 : Fix DCHECK #Patch Set 9 : fix dcheck again #
Total comments: 2
Patch Set 10 : style fix #Patch Set 11 : style fix #Patch Set 12 : Add (failing!) breakpoint tests #Patch Set 13 : Fix tests, add generator tests #Patch Set 14 : fix formatting #Patch Set 15 : formatting again #Patch Set 16 : don't pass unneeded zero #
Created: 4 years, 6 months ago
Messages
Total messages: 69 (33 generated)
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/patch-status/1996943002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/1
littledan@chromium.org changed reviewers: + caitpotter88@gmail.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/15662)
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/patch-status/1996943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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/patch-status/1996943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/40001
Description was changed from ========== [esnext] Fix various callsites to use is_resumable, not is_generator Async functions are built out of generators, but the SharedFunctionInfo returns false for is_generator. is_resumable is the broader query. This patch fixes many parts of V8 to refer to is_resumable as appropriate. BUG=v8:4483 ========== to ========== [esnext] Fix various callsites to use is_resumable, not is_generator Async functions are built out of generators, but the SharedFunctionInfo returns false for is_generator. is_resumable is the broader query. This patch fixes many parts of V8 to refer to is_resumable as appropriate. One incidental change is to remove a check for generators extending classes. This is part of a general check for constructors being the only thing which can extend classes, so it is removed here and the error message for the general case is made more accurate. BUG=v8:4483 ==========
https://codereview.chromium.org/1996943002/diff/1/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/debug/debug.cc#newcode1334 src/debug/debug.cc:1334: bool include_generators = !is_interpreted && shared->is_resumable(); I imagine this fixes what could have been broken debugger tests, had they been written. No generator-specific tests were added in 35c28ce0a742e58346d2dea009428cacd442040d, though. Yang might be able to point out failures corrected by this? https://codereview.chromium.org/1996943002/diff/1/src/debug/liveedit.cc File src/debug/liveedit.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/debug/liveedit.cc#newco... src/debug/liveedit.cc:1675: if (shared->is_resumable()) { Maybe a test similar to generators-debug-liveedit.js is worth having? https://codereview.chromium.org/1996943002/diff/1/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/factory.cc#newcode1333 src/factory.cc:1333: if (function->shared()->is_resumable()) { Based on how generator_object_prototype_map is created, this doesn't look correct to me ``` Handle<JSFunction> object_function(native_context()->object_function()); Handle<Map> generator_object_prototype_map = Map::Create(isolate(), 0); Map::SetPrototype(generator_object_prototype_map, generator_object_prototype); native_context()->set_generator_object_prototype_map( *generator_object_prototype_map); ``` But I don't think this code gets reached without adding an API for creating async functions, so I don't believe this can be verified yet https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes.cc File src/runtime/runtime-classes.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes... src/runtime/runtime-classes.cc:97: DCHECK(!Handle<JSFunction>::cast(super_class)->shared()->is_resumable()); Is this CHECK really needed? if we have this check, why not also a bunch for arrow functions, methods, getter/setters, etc? https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes... src/runtime/runtime-classes.cc:113: NewTypeError(MessageTemplate::kExtendsValueNotConstructor, super_class), awfully similar to my CL from yesterday :p but it's cool
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 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/patch-status/1996943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/60001
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/patch-status/1996943002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/80001
This patch doesn't add all the debugger tests, but it fixes basic issues that should've just been set up this way all along. What if we landed this and turned --harmony-async-await back on for clusterfuzz while, in parallel, writing more exhaustive debugging tests? https://codereview.chromium.org/1996943002/diff/1/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/debug/debug.cc#newcode1334 src/debug/debug.cc:1334: bool include_generators = !is_interpreted && shared->is_resumable(); On 2016/05/20 at 13:35:56, caitp wrote: > I imagine this fixes what could have been broken debugger tests, had they been written. No generator-specific tests were added in 35c28ce0a742e58346d2dea009428cacd442040d, though. Yang might be able to point out failures corrected by this? Seems like this will come up if you add a breakpoint to an async function after an await while the function is actually still awaiting the result. Let's write a test for this (but maybe it can come in a follow-on patch?) https://codereview.chromium.org/1996943002/diff/1/src/debug/liveedit.cc File src/debug/liveedit.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/debug/liveedit.cc#newco... src/debug/liveedit.cc:1675: if (shared->is_resumable()) { On 2016/05/20 at 13:35:56, caitp wrote: > Maybe a test similar to generators-debug-liveedit.js is worth having? Yes https://codereview.chromium.org/1996943002/diff/1/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/factory.cc#newcode1333 src/factory.cc:1333: if (function->shared()->is_resumable()) { On 2016/05/20 at 13:35:56, caitp wrote: > Based on how generator_object_prototype_map is created, this doesn't look correct to me > > ``` > Handle<JSFunction> object_function(native_context()->object_function()); > Handle<Map> generator_object_prototype_map = Map::Create(isolate(), 0); > Map::SetPrototype(generator_object_prototype_map, generator_object_prototype); > native_context()->set_generator_object_prototype_map( > *generator_object_prototype_map); > ``` > > But I don't think this code gets reached without adding an API for creating async functions, so I don't believe this can be verified yet Oh you're right, oops. The new patch asserts that we never get here for async functions. https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes.cc File src/runtime/runtime-classes.cc (right): https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes... src/runtime/runtime-classes.cc:97: DCHECK(!Handle<JSFunction>::cast(super_class)->shared()->is_resumable()); On 2016/05/20 at 13:35:56, caitp wrote: > Is this CHECK really needed? if we have this check, why not also a bunch for arrow functions, methods, getter/setters, etc? Just because I'm removing old code on the basis of it being unreachable; the DCHECK makes sure that it wasn't a mistake. https://codereview.chromium.org/1996943002/diff/1/src/runtime/runtime-classes... src/runtime/runtime-classes.cc:113: NewTypeError(MessageTemplate::kExtendsValueNotConstructor, super_class), On 2016/05/20 at 13:35:56, caitp wrote: > awfully similar to my CL from yesterday :p but it's cool Oops! I hadn't gotten around to reviewing it yet. Anyway worked out a test fix in this patch; I don't think we need new tests.
On 2016/05/20 14:59:19, Dan Ehrenberg wrote: > This patch doesn't add all the debugger tests, but it fixes basic issues that > should've just been set up this way all along. What if we landed this and turned > --harmony-async-await back on for clusterfuzz while, in parallel, writing more > exhaustive debugging tests? I prefer if the debugger tests that are expected to be affected by these changes are added here (particularly the liveedit thing) --- because I find it useful to be able to find tests added for specific blame lines. But it's up to you I guess. About the "breakpoint after await" thing, I believe async-debug-basic.js has that, without any kind of crashing or anything, so yeah, not sure what's missing there.
On 2016/05/20 15:21:38, caitp wrote: > On 2016/05/20 14:59:19, Dan Ehrenberg wrote: > > This patch doesn't add all the debugger tests, but it fixes basic issues that > > should've just been set up this way all along. What if we landed this and > turned > > --harmony-async-await back on for clusterfuzz while, in parallel, writing more > > exhaustive debugging tests? > > I prefer if the debugger tests that are expected to be affected by these changes > are added here (particularly the liveedit thing) --- because I find it useful to > be able to find tests added for specific blame lines. But it's up to you I > guess. > > About the "breakpoint after await" thing, I believe async-debug-basic.js has > that, without any kind of crashing or anything, so yeah, not sure what's missing > there. Otherwise lgtm, fwiw
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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/patch-status/1996943002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...) v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
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/patch-status/1996943002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/6418) v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
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/patch-status/1996943002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/6420) v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
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/patch-status/1996943002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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/patch-status/1996943002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
littledan@chromium.org changed reviewers: + adamk@chromium.org
adamk@chromium.org changed reviewers: + yangguo@chromium.org
+yang This seems like the right way for the code to look, but I do think that this fix ought to have regression tests land along with it.
LGTM. I assume we are going to have some tests that have async functions on the stack when we start stepping and/or set break points? (see test/mjsunit/ignition/debug-break-on-stack). https://codereview.chromium.org/1996943002/diff/160001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/1996943002/diff/160001/src/debug/debug.cc#new... src/debug/debug.cc:1332: bool include_generators = !is_interpreted && shared->is_resumable(); how about we rename "include_generators" to "find_resumable"?
Working on tests. https://codereview.chromium.org/1996943002/diff/160001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/1996943002/diff/160001/src/debug/debug.cc#new... src/debug/debug.cc:1332: bool include_generators = !is_interpreted && shared->is_resumable(); On 2016/05/30 at 12:32:46, Yang wrote: > how about we rename "include_generators" to "find_resumable"? Good idea, done.
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/patch-status/1996943002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/16234)
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/patch-status/1996943002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/16237)
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/patch-status/1996943002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/280001
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/patch-status/1996943002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/300001
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitpotter88@gmail.com, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1996943002/#ps300001 (title: "don't pass unneeded zero")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996943002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996943002/300001
On 2016/05/31 at 16:51:10, commit-bot wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1996943002/300001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1996943002/300001 Committing with some tests which caitp reviewed. No liveedit tests yet; there just aren't any tests for the failure path at all, and there also aren't any tests (besides the ones added in this patch) that tests debugging generators. Caitlin and I will work on these tests and more, based on Yang's suggestion, but would like to have this in so that we can turn back on es-staging for async/await and get more Clusterfuzz feedback.
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [esnext] Fix various callsites to use is_resumable, not is_generator Async functions are built out of generators, but the SharedFunctionInfo returns false for is_generator. is_resumable is the broader query. This patch fixes many parts of V8 to refer to is_resumable as appropriate. One incidental change is to remove a check for generators extending classes. This is part of a general check for constructors being the only thing which can extend classes, so it is removed here and the error message for the general case is made more accurate. BUG=v8:4483 ========== to ========== [esnext] Fix various callsites to use is_resumable, not is_generator Async functions are built out of generators, but the SharedFunctionInfo returns false for is_generator. is_resumable is the broader query. This patch fixes many parts of V8 to refer to is_resumable as appropriate. One incidental change is to remove a check for generators extending classes. This is part of a general check for constructors being the only thing which can extend classes, so it is removed here and the error message for the general case is made more accurate. BUG=v8:4483 Committed: https://crrev.com/46253e743df20c67bd524cf6f4288f549cb72739 Cr-Commit-Position: refs/heads/master@{#36621} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/46253e743df20c67bd524cf6f4288f549cb72739 Cr-Commit-Position: refs/heads/master@{#36621} |