[Kernel] replace function debuggable field with originalAsyncMarker field

[Kernel] replace function debuggable originalAsyncMarker

Previously a functions debuggable field was set to false in the async transformation (and because it wasn't done yet, not in async* and sync* transformation, though it should have been). Now instead, set the original async status and set debuggable on the c++ side equivalently.

Also, on the c++ side, set a functions modifier to async etc if they were originally async etc.

BUG=
R=asgerf@google.com, kmillikin@google.com, kustermann@google.com

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

[jensj, 2017-02-17 13:19:41]

Description was changed from

==========
[Kernel] replace function debuggable field with originalAsyncMarker field

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

to

==========
[Kernel] replace function debuggable originalAsyncMarker

Previously a functions debuggable field was set to false in the async
transformation (and because it wasn't done yet, not in async* and sync*
transformation, though it should have been). Now instead, set the original async
status and set debuggable on the c++ side equivalently.

Also, on the c++ side, set a functions modifier to async etc if they were
originally async etc.

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

[jensj, 2017-02-17 13:19:41]

jensj@google.com changed reviewers:
+ asgerf@google.com, kustermann@google.com

[kustermann, 2017-02-17 13:28:08]

LGTM, thanks for splitting it out.

(Theoretically we could combine the two bytes into one to reduce binary size,
but we can do that later.)

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/text/ast_to_...
File pkg/kernel/lib/text/ast_to_text.dart (right):

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:504:
writeSpaced(getAsyncMarkerKeyword(function.asyncMarker));
asyncMarker -> originalAsyncMarker

[asgerf, 2017-02-17 13:31:31]

lgtm

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1154: AsyncMarker originalAsyncMarker;
Could you please add some documentation, explaining why there are two async
markers?

[jensj, 2017-02-20 09:06:29]

Comments addressed.
Also made a few other changes (e.g. I forgot to update clone.dart) ---
committing anyway. If you disagree tell me and I'll undo.

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1154: AsyncMarker originalAsyncMarker;
On 2017/02/17 13:31:30, asgerf wrote:
> Could you please add some documentation, explaining why there are two async
> markers?

Done.

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/text/ast_to_...
File pkg/kernel/lib/text/ast_to_text.dart (right):

https://codereview.chromium.org/2697193008/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:504:
writeSpaced(getAsyncMarkerKeyword(function.asyncMarker));
On 2017/02/17 13:28:08, kustermann wrote:
> asyncMarker -> originalAsyncMarker

Done.

[jensj, 2017-02-20 09:30:20]

jensj@google.com changed reviewers:
+ kmillikin@google.com

[jensj, 2017-02-20 09:30:43]

So I'll hold off on the commit anyway. Kevin asked to be looped in first.

[Kevin Millikin (Google), 2017-02-22 07:59:59]

Comments below.

It seems warty to have both markers on functions.  I think it would be cleaner
have distinct SyncYielding markers for sync*, async, and async* and then in the
VM set the status of the parent function when a SyncYielding function is seen.

The unfortunate downside of that approach is that we have to deserialize the
parent function's body to set its marker correctly, so this change seems OK.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:326: AsyncStar
Hmm, SyncYielding is missing.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:1158: /// In source code, a function can for instance be
marked as async.
I wouldn't discuss all these imperative details (specifically, one might just
build a new function and this discussion of updating fields doesn't make any
sense).  Instead of "current" and "original" I would adopt naming something like
dartAsyncMarker and kernelAsyncMarker and then make the comment declarative:

"A Kernel function can represent a Dart function with a different async marker."

If we need an example we can write: "For example, when async/await is translated
away, a Dart async function might be represented by a Kernel sync function."

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/continuation.dart (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:149:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
It doesn't make sense to ever change the original async marker.  If we do that,
it might not be the original one (it's just the previous one).

I don't understand why we don't just leave it alone.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:702:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
Same comment.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:825:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
Same comment.

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:6059: // TODO(27590): we need a better way to detect
if we need to check for an
If we set it correctly, can't we instead ask here if
parsed_function_->function() IsAsyncClosure() or IsAsyncGenClosure(), removing
the TODO?

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:6135:
function.set_is_debuggable(node->original_async_marker() ==
Why not set the function to be correctly sync*/async/async*?

We should also (probably) set SyncYielding functions to be is_generated_body so
they get the correct SyncGenClosure/AsyncClosure/AsyncGenClosure status.

[jensj, 2017-02-22 10:36:45]

Comments addressed. PTAL.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:326: AsyncStar
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> Hmm, SyncYielding is missing.

Added.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:1158: /// In source code, a function can for instance be
marked as async.
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> I wouldn't discuss all these imperative details (specifically, one might just
> build a new function and this discussion of updating fields doesn't make any
> sense).  Instead of "current" and "original" I would adopt naming something
like
> dartAsyncMarker and kernelAsyncMarker and then make the comment declarative:
> 
> "A Kernel function can represent a Dart function with a different async
marker."
> 
> If we need an example we can write: "For example, when async/await is
translated
> away, a Dart async function might be represented by a Kernel sync function."

Done.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/continuation.dart (right):

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:149:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> It doesn't make sense to ever change the original async marker.  If we do
that,
> it might not be the original one (it's just the previous one).
> 
> I don't understand why we don't just leave it alone.

Done.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:702:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> Same comment.

Done.

https://codereview.chromium.org/2697193008/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/continuation.dart:825:
enclosingFunction.originalAsyncMarker = enclosingFunction.asyncMarker;
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> Same comment.

Done.

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:6059: // TODO(27590): we need a better way to detect
if we need to check for an
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> If we set it correctly, can't we instead ask here if
> parsed_function_->function() IsAsyncClosure() or IsAsyncGenClosure(), removing
> the TODO?

That makes sence.
I've tried, and it seems to pass all tests.

https://codereview.chromium.org/2697193008/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:6135:
function.set_is_debuggable(node->original_async_marker() ==
On 2017/02/22 07:59:59, Kevin Millikin (Google) wrote:
> Why not set the function to be correctly sync*/async/async*?
> 
> We should also (probably) set SyncYielding functions to be is_generated_body
so
> they get the correct SyncGenClosure/AsyncClosure/AsyncGenClosure status.

Done.

[Kevin Millikin (Google), 2017-02-22 10:46:04]

Thanks, LGTM.

https://codereview.chromium.org/2697193008/diff/40001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2697193008/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:447:
function.set_is_generated_body(kernel_procedure->function()->async_marker() ==
This is safe but we won't see kSyncYielding here so you could also change it to
an ASSERT.

[jensj, 2017-02-22 11:12:36]

Thanks. Landing.

https://codereview.chromium.org/2697193008/diff/40001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2697193008/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:447:
function.set_is_generated_body(kernel_procedure->function()->async_marker() ==
On 2017/02/22 10:46:03, Kevin Millikin (Google) wrote:
> This is safe but we won't see kSyncYielding here so you could also change it
to
> an ASSERT.

Changing to
```
ASSERT(kernel_procedure->function()->async_marker() == FunctionNode::kSync);
```

[jensj, 2017-02-22 11:13:02]

Description was changed from

==========
[Kernel] replace function debuggable originalAsyncMarker

Previously a functions debuggable field was set to false in the async
transformation (and because it wasn't done yet, not in async* and sync*
transformation, though it should have been). Now instead, set the original async
status and set debuggable on the c++ side equivalently.

Also, on the c++ side, set a functions modifier to async etc if they were
originally async etc.

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

to

==========
[Kernel] replace function debuggable originalAsyncMarker

Previously a functions debuggable field was set to false in the async
transformation (and because it wasn't done yet, not in async* and sync*
transformation, though it should have been). Now instead, set the original async
status and set debuggable on the c++ side equivalently.

Also, on the c++ side, set a functions modifier to async etc if they were
originally async etc.

BUG=
R=asgerf@google.com, kmillikin@google.com, kustermann@google.com

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

[jensj, 2017-02-22 11:13:03]

Committed patchset #4 (id:60001) manually as
86d788e6994bf9e7b8f93d2132cd1099d60229ec (presubmit successful).