Mark await expressions as caught or uncaught

Mark await expressions as caught or uncaught

Handle some examples of the "asynchronous case" by marking await expressions
as either caught or uncaught; in the caught case, this marks the Promise passed
in as having a catch predicted. The marking is done in AST numbering, which
chooses between two different runtime function calls based on catch prediction.

BUG=v8:5167
Committed: https://crrev.com/edb4d3151ce963c531327bd2345008b2e108913d
Cr-Commit-Position: refs/heads/master@{#39394}

[Dan Ehrenberg, 2016-08-25 14:15:21]

Description was changed from

==========
Fix two aspects of async/await ExceptionEvents

- Handle the "synchronous case" by
- Handle some examples of the "asynchronous case" by marking ...

BUG=
==========

to

==========
Fix two aspects of async/await ExceptionEvents

- Handle the "synchronous case" by
- Handle some examples of the "asynchronous case" by marking ...

BUG=v8:5167
==========

[Dan Ehrenberg, 2016-09-02 16:56:26]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 16:56:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-02 17:01:53]

Description was changed from

==========
Fix two aspects of async/await ExceptionEvents

- Handle the "synchronous case" by
- Handle some examples of the "asynchronous case" by marking ...

BUG=v8:5167
==========

to

==========
Fix two cases for async/await catch prediction

- Handle the "synchronous case" by marking try/catch blocks introduced for
  async functions as ASYNC_AWAIT and traversing up the stack, finding successive
  Promises and returning caught if any of them are predicted to be caught.
- Handle some examples of the "asynchronous case" by marking await expressions
  as either caught or uncaught; in the caught case, this marks the Promise
passed
  in as having a catch predicted. The marking is done in AST numbering, which
  chooses between two different runtime function calls based on catch
prediction.

BUG=v8:5167
==========

[Dan Ehrenberg, 2016-09-02 17:01:53]

littledan@chromium.org changed reviewers:
+ jgruber@chromium.org, neis@chromium.org

[Dan Ehrenberg, 2016-09-02 17:02:20]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org, caitp@igalia.com

[commit-bot: I haz the power, 2016-09-02 17:13:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 17:13:21]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Dan Ehrenberg, 2016-09-02 17:16:29]

littledan@chromium.org changed reviewers:
- adamk@chromium.org, caitp@igalia.com

[Dan Ehrenberg, 2016-09-02 17:21:45]

littledan@chromium.org changed reviewers:
- jgruber@chromium.org, neis@chromium.org

[Dan Ehrenberg, 2016-09-07 22:27:50]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:27:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-07 22:27:56]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:29:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:29:25]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  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_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23556)

[Dan Ehrenberg, 2016-09-07 22:30:28]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:30:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-07 22:32:15]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:32:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:38:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:38:47]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23559)

[Dan Ehrenberg, 2016-09-08 01:11:58]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org, jgruber@chromium.org

[Dan Ehrenberg, 2016-09-08 01:30:56]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 01:31:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-08 01:44:09]

littledan@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org

[Dan Ehrenberg, 2016-09-08 01:53:59]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 01:54:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 02:18:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 02:18:16]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-08 10:19:59]

+mstarzinger for ast-numbering changes.

Looking good. Would it perhaps be possible to split the two cases into separate
CLs? Some comments:

https://codereview.chromium.org/2276243002/diff/100001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2276243002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:1705: catch_type == Isolate::CAUGHT_BY_ASYNC_AWAIT);
Could you add a comment about why CAUGHT_BY_ASYNC_AWAIT means the exception is
uncaught?

https://codereview.chromium.org/2276243002/diff/100001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/100001/src/js/harmony-async-a...
src/js/harmony-async-await.js:14: var InternalArray = utils.InternalArray;
Unused.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-debug-caught-exception2.js (right):

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:9: let exception = null;
This is never set.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:15: events++;
Can this ever fail?

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:67: async function
caught(producer) {
Duplicate from above.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:110: let lateCatches =
[dotCatch, indirectReturnDotCatch, indirectAwaitDotCatch, nestedDotCatch];
Nit: 80 columns here and below.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:116: // let lateThrows =
[awaitThrow, constructorThrow, rejectConstructor];
Maybe we could add these (awaitThrow, constructorThrow, rejectConstructor,
lateThrows, the for loop below and the comment) once they become relevant?
Otherwise it's just dead code here. But I guess I'm fine with either way.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:158: function
afterWaiting(callback, tasks = tonsOfTasks) {
This appears to be unused.

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:188: if (failureCount > 0)
{
What about assertEquals() here (or above in the loop)?

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:256:
node->set_context_index(Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT_INDEX);
Changing the called runtime function from the AstNumberingVisitor seems very
unexpected to me. At the very least there should be a huge comment about this
where the runtime call is created in the parser. 
But maybe someone else should weigh in here that knows more about this than me.

Is it not possible to do this from the parser?

[Dan Ehrenberg, 2016-09-08 18:24:46]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 18:27:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 18:27:05]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-08 18:58:36]

Still having trouble paging all this back into my head so I'm going to mostly
defer to the other reviewers, but some comments below. I'm with jgruber that I'd
like to better understand why the choice has to happen in AstNumbering.

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast.h#newcode1989
src/ast/ast.h:1989: int set_context_index(int index) {
"void" is the usual return type for setters, seems like this one could be void
too.

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.cc#old...
src/debug/debug.cc:1718: ASSIGN_RETURN_ON_EXCEPTION_VALUE(
Previously we would return here if we had an exception, but now we continue on.
But I'm not sure if I understand when we could have an exception there...do you?

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.h#newc...
src/debug/debug.h:416: void OnPromiseReject(Handle<Object> promise,
Handle<Object> value);
Was this just wrong before?

https://codereview.chromium.org/2276243002/diff/120001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/120001/src/js/harmony-async-a...
src/js/harmony-async-await.js:14: var InternalArray = utils.InternalArray;
Nit: alphabetize?

https://codereview.chromium.org/2276243002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/objects.h#newcode4656
src/objects.h:4656: class HandlerOffsetField : public BitField<int, 3, 29> {};
Sorry I'm not familiar here, what is this an offset into? i.e., is 29 bits
sufficient?

[Dan Ehrenberg, 2016-09-08 20:13:15]

To answer the question of why the caught/uncaught marking happens in
ast-numbering: This is when catch prediction currently occurs, and it is the
only common point that has access to this information. The parser just doesn't
know yet. Take this case of catch prediction:

try {
  await fn()
} catch (e) {
}

vs

try {
  await fn();
} finally {
}

When parsing the await that we want to mark as caught or uncaught, it's not yet
known whether it will be followed by a 'finally' or a 'catch.

ast-numbering is what learns whether it is caught. To make the information
available later, ast-numbering has to put it somewhere. The handler tables can
be read at runtime, but are not constructed at this point, I believe. Therefore,
it seemed like it had to be marked somewhere else. Changing the runtime function
into another one in ast-numbering seemed like a simple and straightforward
solution to that problem.

[Dan Ehrenberg, 2016-09-08 22:29:50]

Description was changed from

==========
Fix two cases for async/await catch prediction

- Handle the "synchronous case" by marking try/catch blocks introduced for
  async functions as ASYNC_AWAIT and traversing up the stack, finding successive
  Promises and returning caught if any of them are predicted to be caught.
- Handle some examples of the "asynchronous case" by marking await expressions
  as either caught or uncaught; in the caught case, this marks the Promise
passed
  in as having a catch predicted. The marking is done in AST numbering, which
  chooses between two different runtime function calls based on catch
prediction.

BUG=v8:5167
==========

to

==========
Mark await expressions as caught or uncaught

Handle some examples of the "asynchronous case" by marking await expressions
as either caught or uncaught; in the caught case, this marks the Promise passed
in as having a catch predicted. The marking is done in AST numbering, which
chooses between two different runtime function calls based on catch prediction.

BUG=v8:5167
==========

[Dan Ehrenberg, 2016-09-08 22:34:05]

I split out this patch into two; the other one is at
https://codereview.chromium.org/2325813002

https://codereview.chromium.org/2276243002/diff/100001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/100001/src/js/harmony-async-a...
src/js/harmony-async-await.js:14: var InternalArray = utils.InternalArray;
On 2016/09/08 at 10:19:58, jgruber wrote:
> Unused.

Removed

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-debug-caught-exception2.js (right):

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:9: let exception = null;
On 2016/09/08 at 10:19:58, jgruber wrote:
> This is never set.

Deleted

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:15: events++;
On 2016/09/08 at 10:19:58, jgruber wrote:
> Can this ever fail?

Deleted

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:67: async function
caught(producer) {
On 2016/09/08 at 10:19:58, jgruber wrote:
> Duplicate from above.

Deleted

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:110: let lateCatches =
[dotCatch, indirectReturnDotCatch, indirectAwaitDotCatch, nestedDotCatch];
On 2016/09/08 at 10:19:58, jgruber wrote:
> Nit: 80 columns here and below.

Fixed

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:116: // let lateThrows =
[awaitThrow, constructorThrow, rejectConstructor];
On 2016/09/08 at 10:19:58, jgruber wrote:
> Maybe we could add these (awaitThrow, constructorThrow, rejectConstructor,
lateThrows, the for loop below and the comment) once they become relevant?
Otherwise it's just dead code here. But I guess I'm fine with either way.

Done

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:116: // let lateThrows =
[awaitThrow, constructorThrow, rejectConstructor];
On 2016/09/08 at 10:19:58, jgruber wrote:
> Maybe we could add these (awaitThrow, constructorThrow, rejectConstructor,
lateThrows, the for loop below and the comment) once they become relevant?
Otherwise it's just dead code here. But I guess I'm fine with either way.

Done

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:158: function
afterWaiting(callback, tasks = tonsOfTasks) {
On 2016/09/08 at 10:19:58, jgruber wrote:
> This appears to be unused.

Removed

https://codereview.chromium.org/2276243002/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-debug-caught-exception2.js:188: if (failureCount > 0)
{
On 2016/09/08 at 10:19:58, jgruber wrote:
> What about assertEquals() here (or above in the loop)?

Now it exits at the first failure (though this form was useful for development)

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/ast/ast.h#newcode1989
src/ast/ast.h:1989: int set_context_index(int index) {
On 2016/09/08 at 18:58:35, adamk wrote:
> "void" is the usual return type for setters, seems like this one could be void
too.

Fixed

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.cc#old...
src/debug/debug.cc:1718: ASSIGN_RETURN_ON_EXCEPTION_VALUE(
On 2016/09/08 at 18:58:35, adamk wrote:
> Previously we would return here if we had an exception, but now we continue
on. But I'm not sure if I understand when we could have an exception there...do
you?

An exception in this code would mean a bug in src/js/promise.js. That function
is not supposed to throw an exception. This patch will make that case result in
a misprediction that the exception is uncaught rather than ignoring the
exception. I think it's better to err on the side of more reports, but anyway, I
don't expect (or hope) that this case will come up in practice. Another option
would be to crash if an exception is thrown, if we are worried about suppressing
these errors.

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/debug/debug.h#newc...
src/debug/debug.h:416: void OnPromiseReject(Handle<Object> promise,
Handle<Object> value);
On 2016/09/08 at 18:58:36, adamk wrote:
> Was this just wrong before?

No, I weakened it so that it can also accept 'undefined', which can be used as
an indication that there's no relevant Promise to traverse up through. This
seemed to be the cleanest way to pass that state up through the program. It's
explained in a comment in src/runtime/runtime-internal.cc line 311.

https://codereview.chromium.org/2276243002/diff/120001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/120001/src/js/harmony-async-a...
src/js/harmony-async-await.js:14: var InternalArray = utils.InternalArray;
On 2016/09/08 at 18:58:36, adamk wrote:
> Nit: alphabetize?

Deleted

https://codereview.chromium.org/2276243002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2276243002/diff/120001/src/objects.h#newcode4656
src/objects.h:4656: class HandlerOffsetField : public BitField<int, 3, 29> {};
On 2016/09/08 at 18:58:36, adamk wrote:
> Sorry I'm not familiar here, what is this an offset into? i.e., is 29 bits
sufficient?

This is for a particular code object. I don't think it'll get anywhere near this
big. Anyway, if our tests hit it, it'll cause a DCHECK failure.

[jgruber, 2016-09-09 07:22:40]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 07:22:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 07:39:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 07:39:20]

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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Dan Ehrenberg, 2016-09-09 17:30:38]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:30:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:05:28]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-09 18:46:29]

This is looking good, but I'd find the logic a lot easier to understand with
some comments added

https://codereview.chromium.org/2276243002/diff/160001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/160001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:259: // try { await fn(); } finally { }
Nit: remove the semicolon, it's distracting since it suggests that that's one of
the differences between the first and second.

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:52: function AsyncFunctionAwait(generator,
awaited, mark) {
Comment please

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:76: function AsyncFunctionAwaitUncaught(generator,
awaited) {
Comment please

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:81: function AsyncFunctionAwaitCaught(generator,
awaited) {
And here.

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:540: if (GET_PRIVATE(handler, promiseAwaitHandlerSymbol))
return false;
I would appreciate a comment here.

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:554: var handledHintSymbol = GET_PRIVATE(promise,
promiseHandledHintSymbol);
Why store the return value of GET_PRIVATE?

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:555: if (handledHintSymbol) return true;
A comment here too please.

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:561: } else {
I'd say just drop this else rather than moving the return into the block.

[Dan Ehrenberg, 2016-09-10 01:54:08]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[Dan Ehrenberg, 2016-09-10 01:54:14]

https://codereview.chromium.org/2276243002/diff/160001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/160001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:259: // try { await fn(); } finally { }
On 2016/09/09 at 18:46:28, adamk wrote:
> Nit: remove the semicolon, it's distracting since it suggests that that's one
of the differences between the first and second.

Added the semicolon to the other case

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:52: function AsyncFunctionAwait(generator,
awaited, mark) {
On 2016/09/09 at 18:46:28, adamk wrote:
> Comment please

Done

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:76: function AsyncFunctionAwaitUncaught(generator,
awaited) {
On 2016/09/09 at 18:46:28, adamk wrote:
> Comment please

Done

https://codereview.chromium.org/2276243002/diff/160001/src/js/harmony-async-a...
src/js/harmony-async-await.js:81: function AsyncFunctionAwaitCaught(generator,
awaited) {
On 2016/09/09 at 18:46:28, adamk wrote:
> And here.

Done

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:540: if (GET_PRIVATE(handler, promiseAwaitHandlerSymbol))
return false;
On 2016/09/09 at 18:46:28, adamk wrote:
> I would appreciate a comment here.

Done

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:554: var handledHintSymbol = GET_PRIVATE(promise,
promiseHandledHintSymbol);
On 2016/09/09 at 18:46:29, adamk wrote:
> Why store the return value of GET_PRIVATE?

Removed

https://codereview.chromium.org/2276243002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:555: if (handledHintSymbol) return true;
On 2016/09/09 at 18:46:28, adamk wrote:
> A comment here too please.

Done

[commit-bot: I haz the power, 2016-09-10 01:54:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 01:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 01:55:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8587)

[Dan Ehrenberg, 2016-09-10 02:19:59]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 02:20:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 02:58:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 02:58:34]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-12 09:29:00]

Thanks for splitting this out, it's much more readable now.

Could you still add relevant tests?

LGTM modulo comments.

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:271:
node->set_context_index(Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT_INDEX);
This still raises flags for me. I understand why it can't be done in the parser,
but what about e.g. creating a new visitor just for catch prediction and
rewriting? 

But if no one else objects I'll concur.

https://codereview.chromium.org/2276243002/diff/200001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/2276243002/diff/200001/src/js/harmony-async-a...
src/js/harmony-async-await.js:54: // Shared logic for the core of await. The
parser desugars
Thanks, these comments are very helpful.

https://codereview.chromium.org/2276243002/diff/200001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/200001/src/js/promise.js#newc...
src/js/promise.js:540: // If this handler was installed by async/await, it does
not indicate
Do you mean 'it indicates that there is no user-defined reject handler'?

[jgruber, 2016-09-12 12:22:27]

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:271:
node->set_context_index(Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT_INDEX);
On 2016/09/12 09:29:00, jgruber wrote:
> This still raises flags for me. I understand why it can't be done in the
parser,
> but what about e.g. creating a new visitor just for catch prediction and
> rewriting? 
> 
> But if no one else objects I'll concur.

After speaking with Nikolaos about this, a new visitor might be a bad idea due
to performance reasons. Let's land it like this for now and clean up later
(possibly with the NonPatternRewriter?).

[Dan Ehrenberg, 2016-09-12 14:07:46]

Tests are added in the two follow-on patches.

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering....
src/ast/ast-numbering.cc:271:
node->set_context_index(Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT_INDEX);
On 2016/09/12 at 12:22:27, jgruber wrote:
> On 2016/09/12 09:29:00, jgruber wrote:
> > This still raises flags for me. I understand why it can't be done in the
parser,
> > but what about e.g. creating a new visitor just for catch prediction and
> > rewriting? 
> > 
> > But if no one else objects I'll concur.
> 
> After speaking with Nikolaos about this, a new visitor might be a bad idea due
to performance reasons. Let's land it like this for now and clean up later
(possibly with the NonPatternRewriter?).

Catch prediction runs in the AstNumberingVisitor, which is after all the
rewriters. It could hypothetically be moved there, but I don't see how that
would make anything cleaner.

https://codereview.chromium.org/2276243002/diff/200001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/200001/src/js/promise.js#newc...
src/js/promise.js:540: // If this handler was installed by async/await, it does
not indicate
On 2016/09/12 at 09:29:00, jgruber wrote:
> Do you mean 'it indicates that there is no user-defined reject handler'?

No; one of the other recursive calls may return true. This false ends up being
or-ed together with other results.

https://codereview.chromium.org/2276243002/diff/200001/src/js/promise.js#newc...
src/js/promise.js:540: // If this handler was installed by async/await, it does
not indicate
On 2016/09/12 at 09:29:00, jgruber wrote:
> Do you mean 'it indicates that there is no user-defined reject handler'?

No; one of the other recursive calls may return true. This false ends up being
or-ed together with other results.

[adamk, 2016-09-12 15:58:25]

Thanks, the comments are helpful, one more question below.

As for the ast-numbering thing: given where catch prediction is, that seems like
the "right" place for now (the other easy option would be to have all the
backends swap it out, but that seems scarier).

It seems like the AstNumberingVisitor probably could use renaming, though (not
in this CL of course).

https://codereview.chromium.org/2276243002/diff/180001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/180001/src/js/promise.js#newc...
src/js/promise.js:540: // If this handler was installed by async/await, it does
not indicate
Do you mean "it indicates that there is not"? The comment as-is doesn't explain
why this is an early return.

[kozy, 2016-09-12 17:14:45]

lgtm for marking logic from DevTools point of view.
Could you add a test?

[Dan Ehrenberg, 2016-09-12 21:50:06]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:50:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 22:09:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:09:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Dan Ehrenberg, 2016-09-12 22:10:46]

On 2016/09/12 at 12:22:27, jgruber wrote:
>
https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering.cc
> File src/ast/ast-numbering.cc (right):
> 
>
https://codereview.chromium.org/2276243002/diff/200001/src/ast/ast-numbering....
> src/ast/ast-numbering.cc:271:
node->set_context_index(Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT_INDEX);
> On 2016/09/12 09:29:00, jgruber wrote:
> > This still raises flags for me. I understand why it can't be done in the
parser,
> > but what about e.g. creating a new visitor just for catch prediction and
> > rewriting? 
> > 
> > But if no one else objects I'll concur.
> 
> After speaking with Nikolaos about this, a new visitor might be a bad idea due
to performance reasons. Let's land it like this for now and clean up later
(possibly with the NonPatternRewriter?).

It's hard for me to picture how it should be cleaned up, besides renaming the
AstNumberingVisitor. It seems pretty clean to me that that's the stage where
catch prediction happens.

[Dan Ehrenberg, 2016-09-12 22:11:50]

Working on a test (though the one that's uploaded fails). It's a bit funny to
develop a test for an intermediate point; the following two patches introduce
tests that exercise this code path.

https://codereview.chromium.org/2276243002/diff/180001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2276243002/diff/180001/src/js/promise.js#newc...
src/js/promise.js:540: // If this handler was installed by async/await, it does
not indicate
On 2016/09/12 at 15:58:25, adamk wrote:
> Do you mean "it indicates that there is not"? The comment as-is doesn't
explain why this is an early return.

No, it does not indicate that there is. This is just one step in a graph
traversal; other subgraphs may find a handler.

[Dan Ehrenberg, 2016-09-12 23:42:54]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 23:43:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-09-12 23:51:21]

I don't think there's a good way to observe the caught exception with just this
one patch. However, I think there's value in separating this patch from the
other. Could we submit it separately, depending on the following patches for
tests?

[commit-bot: I haz the power, 2016-09-13 00:15:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 00:15:52]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-09-13 20:46:16]

On 2016/09/12 23:51:21, Dan Ehrenberg wrote:
> I don't think there's a good way to observe the caught exception with just
this
> one patch. However, I think there's value in separating this patch from the
> other. Could we submit it separately, depending on the following patches for
> tests?

I don't mind but please wait for other reviewers.

[adamk, 2016-09-13 20:47:05]

lgtm from my end

[Dan Ehrenberg, 2016-09-13 20:47:30]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-09-13 20:47:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2276243002/#ps240001
(title: "Revert "Tests for caught exception"")

[commit-bot: I haz the power, 2016-09-13 20:47:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 20:50:02]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-09-13 20:51:14]

Description was changed from

==========
Mark await expressions as caught or uncaught

Handle some examples of the "asynchronous case" by marking await expressions
as either caught or uncaught; in the caught case, this marks the Promise passed
in as having a catch predicted. The marking is done in AST numbering, which
chooses between two different runtime function calls based on catch prediction.

BUG=v8:5167
==========

to

==========
Mark await expressions as caught or uncaught

Handle some examples of the "asynchronous case" by marking await expressions
as either caught or uncaught; in the caught case, this marks the Promise passed
in as having a catch predicted. The marking is done in AST numbering, which
chooses between two different runtime function calls based on catch prediction.

BUG=v8:5167

Committed: https://crrev.com/edb4d3151ce963c531327bd2345008b2e108913d
Cr-Commit-Position: refs/heads/master@{#39394}
==========

[commit-bot: I haz the power, 2016-09-13 20:51:15]

Patchset 13 (id:??) landed as
https://crrev.com/edb4d3151ce963c531327bd2345008b2e108913d
Cr-Commit-Position: refs/heads/master@{#39394}