Dart VM: Fix an order-of-evaluation bug in async code.

Dart VM: Fix an order-of-evaluation bug in async code.

Reading and writing local and static variables is a side effect that
might be observable from a Future, so such reads and writes cannot be
scheduled after the Future completes when they should be performed
before.

The same is true for throwing an exception.

Closes https://github.com/dart-lang/sdk/issues/26175

R=hausner@google.com
BUG=

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

[Kevin Millikin (Google), 2016-04-07 10:20:11]

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer.cc
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:230: result_ = new(Z) LoadLocalNode(token_pos,
result);
I can't quite call Name(...) here because the token positions don't match.  They
are sufficiently complicated in this function (a mix of node->token_pos(), the
constant token_pos = ST(node->token_pos(), and fresh applications of
ST(node->token_pos)) that I didn't want to do anything that might change it.

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:485: result_ = Name(node);
This, VisitStoreLocalNode, VisitLoadStaticFieldNode, VisitStoreStaticFieldNode,
and VisitThrow are the bug fixes.

[hausner, 2016-04-07 17:12:59]

LGTM.

What throws me off a bit is that you use the noun name as a verb in Name().
MakeNamedValue() is definitely not as elegant...

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer.cc
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:230: result_ = new(Z) LoadLocalNode(token_pos,
result);
On 2016/04/07 10:20:11, Kevin Millikin (Google) wrote:
> I can't quite call Name(...) here because the token positions don't match. 
They
> are sufficiently complicated in this function (a mix of node->token_pos(), the
> constant token_pos = ST(node->token_pos(), and fresh applications of
> ST(node->token_pos)) that I didn't want to do anything that might change it.

The mix of various token position values is indeed confusing here. There was
some recent churn on token_pos stuff (the introduction of synthetic positions
created with ST(), for example). As you observe, token_pos is the same as
ST(node->token_pos()), so it seems to me that:

result_ = Name(new(Z) LoadLocalNode(token_pos, result_param)); 

Would do the same as lines 228-230.  ST(x) is idempotent, so there's no harm in
applying ST() to a value that's already synthetic. Maybe I'm missing something.

https://codereview.chromium.org/1861963007/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:485: result_ = Name(node);
On 2016/04/07 10:20:10, Kevin Millikin (Google) wrote:
> This, VisitStoreLocalNode, VisitLoadStaticFieldNode,
VisitStoreStaticFieldNode,
> and VisitThrow are the bug fixes.

Yes. I removed the equivalent code some time ago in a misguided attempt to
optimize away what I considered to be unnecessary load-store operations.

[Kevin Millikin (Google), 2016-04-08 07:40:25]

Description was changed from

==========
Dart VM: Fix an order-of-evaluation bug in async code.

Reading and writing local and static variables is a side effect that
might be observable from a Future, so such reads and writes cannot be
scheduled after the Future completes when they should be performed
before.

The same is true for throwing an exception.

Closes https://github.com/dart-lang/sdk/issues/26175

R=hausner@google.com
BUG=
==========

to

==========
Dart VM: Fix an order-of-evaluation bug in async code.

Reading and writing local and static variables is a side effect that
might be observable from a Future, so such reads and writes cannot be
scheduled after the Future completes when they should be performed
before.

The same is true for throwing an exception.

Closes https://github.com/dart-lang/sdk/issues/26175

R=hausner@google.com
BUG=

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

[Kevin Millikin (Google), 2016-04-08 07:40:28]

Committed patchset #3 (id:40001) manually as
6f3707f3c18029874baa8f9336c6406abc84b7dd (presubmit successful).

[Kevin Millikin (Google), 2016-04-08 07:41:16]

I changed 'Name' to 'MakeName'.  I'll simplify the token positions as a separate
CL.