Track the 'awaiter return' call stack use it to detect uncaught exceptions in async functions

Track the 'awaiter return' call stack use it to detect uncaught exceptions in async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/cba7e3e79adf41814060119bb61faff7bb658865

[Cutch, 2017-02-14 16:49:36]

Description was changed from

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions
==========

to

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
==========

[Cutch, 2017-02-21 21:11:15]

johnmccutchan@google.com changed reviewers:
+ hausner@google.com, rmacnak@google.com

[Cutch, 2017-02-21 21:12:21]

Description was changed from

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
==========

to

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index
==========

[Cutch, 2017-02-22 15:35:47]

Description was changed from

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index
==========

to

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242
==========

[kevmoo, 2017-02-22 15:59:37]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2017-02-22 15:59:37]

So it wasn't a simple fix? :-)

Thanks so much!

[rmacnak, 2017-02-23 17:50:56]

Some initial thoughts

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:219: Object _awaiter;
/// The closure implementing the async[*]-body that is `await`ing this future.
Function _awaiter;

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:223: Object _awaiter;
"

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:224: Object _generator;
/// The closure implementing the async-generator body that is creating events
for this stream.

https://codereview.chromium.org/2692803006/diff/60001/runtime/observatory/tes...
File
runtime/observatory/tests/service/pause_on_unhandled_async_exceptions_test.dart
(right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/service/pause_on_unhandled_async_exceptions_test.dart:27:
testeeMain() async {
Also test the case with no enclosing try at all, not just one with a
non-matching type.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:546: #if !defined(PRODUCT)
if (FLAG_support_debugger)

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:717: if (fp() == 0) {
if (!live_frame) {
  ...
} else {
  ASSERT(fp != 0)
  ...
}

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:776: if (fp() == 0) {
live_frame

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:836: bool ActivationFrame::HandlesException(const
Instance& exc_obj) {
How much slower is ParserCombinators with the debugger open?

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:837: ExceptionHandlers& handlers =
ExceptionHandlers::Handle();
Move handle creation below the fast exit.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2133: OS::PrintErr("%s is caught by frame %s\n",
exc.ToCString(),
if (FLAG_verbose_debug)

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1144: NOT_IN_PRECOMPILED(RawArray*
await_token_positions_);
Not in this CL, but many of these should really be excluded from product-mode
JIT as well.

[Cutch, 2017-02-24 20:19:21]

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:219: Object _awaiter;
On 2017/02/23 17:50:55, rmacnak wrote:
> /// The closure implementing the async[*]-body that is `await`ing this future.
> Function _awaiter;

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:223: Object _awaiter;
On 2017/02/23 17:50:55, rmacnak wrote:
> "

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/lib/async_patch...
runtime/lib/async_patch.dart:224: Object _generator;
On 2017/02/23 17:50:55, rmacnak wrote:
> /// The closure implementing the async-generator body that is creating events
> for this stream.

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:546: #if !defined(PRODUCT)
On 2017/02/23 17:50:56, rmacnak wrote:
> if (FLAG_support_debugger)

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:717: if (fp() == 0) {
On 2017/02/23 17:50:56, rmacnak wrote:
> if (!live_frame) {
>   ...
> } else {
>   ASSERT(fp != 0)
>   ...
> }

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:776: if (fp() == 0) {
On 2017/02/23 17:50:56, rmacnak wrote:
> live_frame

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:837: ExceptionHandlers& handlers =
ExceptionHandlers::Handle();
On 2017/02/23 17:50:56, rmacnak wrote:
> Move handle creation below the fast exit.

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2133: OS::PrintErr("%s is caught by frame %s\n",
exc.ToCString(),
On 2017/02/23 17:50:56, rmacnak wrote:
> if (FLAG_verbose_debug)

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1144: NOT_IN_PRECOMPILED(RawArray*
await_token_positions_);
On 2017/02/23 17:50:56, rmacnak wrote:
> Not in this CL, but many of these should really be excluded from product-mode
> JIT as well.

Acknowledged.

[Cutch, 2017-02-24 20:19:22]

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:717: if (fp() == 0) {
On 2017/02/23 17:50:56, rmacnak wrote:
> if (!live_frame) {
>   ...
> } else {
>   ASSERT(fp != 0)
>   ...
> }

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:776: if (fp() == 0) {
On 2017/02/23 17:50:56, rmacnak wrote:
> live_frame

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2133: OS::PrintErr("%s is caught by frame %s\n",
exc.ToCString(),
On 2017/02/23 17:50:56, rmacnak wrote:
> if (FLAG_verbose_debug)

Done.

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2692803006/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1144: NOT_IN_PRECOMPILED(RawArray*
await_token_positions_);
On 2017/02/23 17:50:56, rmacnak wrote:
> Not in this CL, but many of these should really be excluded from product-mode
> JIT as well.

Acknowledged.

[rmacnak, 2017-02-27 18:17:09]

lgtm

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:1977: StackFrameIterator iterator(false);
false -> kDontValidateFrames

[Cutch, 2017-02-27 22:15:33]

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:1977: StackFrameIterator iterator(false);
On 2017/02/27 18:17:09, rmacnak wrote:
> false -> kDontValidateFrames

Done.

[Cutch, 2017-02-27 22:16:22]

Description was changed from

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242
==========

to

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/cba7e3e79adf41814060119bb61faff7bb658865
==========

[Cutch, 2017-02-27 22:16:23]

Committed patchset #7 (id:120001) manually as
cba7e3e79adf41814060119bb61faff7bb658865 (presubmit successful).

[Kevin Millikin (Google), 2017-02-28 14:56:13]

Description was changed from

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/cba7e3e79adf41814060119bb61faff7bb658865
==========

to

==========
Track the 'awaiter return' call stack use it to detect uncaught exceptions in
async functions

Tracking the awaiter return call stack:

- [x] Each async function closure now knows who is awaiting on their
return. This is effectively the asynchronous equivalent of the 'frame pointer'.
- [x] Each async* function closure now knows how is listening on their
stream. This is effectively the asynchronous equivalent of the 'frame pointer'.

Detecting uncaught exceptions in async functions:

- [x] Code object keeps a map from :await_jump_var to token position
- [x] Exception Handlers keep track if they are generated (as part of
compilation) or directly from user code
- [x] Debugger maps :await_jump_var to a specific try index

Fixes #27242

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/cba7e3e79adf41814060119bb61faff7bb658865
==========

[hausner, 2017-02-28 19:04:51]

A few post-submit comments.

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:12500: TokenPosition token_pos,
token_pos appears to be unused.

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
runtime/lib/async_patch.dart:72: if (object is! _StreamImpl) {
Curious: why not simply

if (object is _StreamImpl) {
  object._awaiter = awaiter;
}

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
runtime/lib/async_patch.dart:92: if (local is! _StreamImpl) {
ditto

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:721: intptr_t var_desc_len = var_descriptors_.Length();
This is a duplicate variable definition that shadows the outer one.

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:732: } else {
Can't these two cases be unified? When the entry with the right name is found,
then assert that if it's not a live frame, the variable must be in the context.

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:781: intptr_t var_desc_len = var_descriptors_.Length();
Same comments as in GetAsyncCompleter() above

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:855: while (try_index >= try_index_threshold) {
I realize this is a direct transformation from the old code that used the
literal 0, but why not

while (try_index != CatchClauseNode::kInvalidTryIndex) { ...

This seems more appropriate, because the code really doesn't care about the fact
that 0 is the index of an implicit try. (In fact, in non-async code, index 0 can
be a user-defined try index.)

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/symbols.h#n...
runtime/vm/symbols.h:49: V(Controller, ":controller")                           
                     \
In other cases, the internal name is called ColonXXX. Maybe do this there too to
avoid Controller2 name?

[Cutch, 2017-02-28 21:46:53]

See: https://codereview.chromium.org/2720723006

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2692803006/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:12500: TokenPosition token_pos,
On 2017/02/28 19:04:51, hausner wrote:
> token_pos appears to be unused.

Yes, this was part of development and I may use it in the future if my current
search for exception handler logic based on token position (and not PC) doesn't
hold up.

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
runtime/lib/async_patch.dart:72: if (object is! _StreamImpl) {
On 2017/02/28 19:04:51, hausner wrote:
> Curious: why not simply
> 
> if (object is _StreamImpl) {
>   object._awaiter = awaiter;
> }

I was just copying the _awaitHelper pattern :)

https://codereview.chromium.org/2692803006/diff/120001/runtime/lib/async_patc...
runtime/lib/async_patch.dart:92: if (local is! _StreamImpl) {
On 2017/02/28 19:04:51, hausner wrote:
> ditto

ditto

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:721: intptr_t var_desc_len = var_descriptors_.Length();
On 2017/02/28 19:04:51, hausner wrote:
> This is a duplicate variable definition that shadows the outer one.

Good catch!

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:732: } else {
On 2017/02/28 19:04:51, hausner wrote:
> Can't these two cases be unified? When the entry with the right name is found,
> then assert that if it's not a live frame, the variable must be in the
context.

Done.

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:781: intptr_t var_desc_len = var_descriptors_.Length();
On 2017/02/28 19:04:51, hausner wrote:
> Same comments as in GetAsyncCompleter() above

I've actually made a helper function which these two call passing in the Symbol.

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:855: while (try_index >= try_index_threshold) {
On 2017/02/28 19:04:51, hausner wrote:
> I realize this is a direct transformation from the old code that used the
> literal 0, but why not
> 
> while (try_index != CatchClauseNode::kInvalidTryIndex) { ...
> 
> This seems more appropriate, because the code really doesn't care about the
fact
> that 0 is the index of an implicit try. (In fact, in non-async code, index 0
can
> be a user-defined try index.)

This code used to care but then I made further changes. I've gone with your
suggestion and changed it to be != CatchClauseNode::kInvalidTryIndex.

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/2692803006/diff/120001/runtime/vm/symbols.h#n...
runtime/vm/symbols.h:49: V(Controller, ":controller")                           
                     \
On 2017/02/28 19:04:51, hausner wrote:
> In other cases, the internal name is called ColonXXX. Maybe do this there too
to
> avoid Controller2 name?

Done.