Fix a bug in graph construction for for loops.

Fix a bug in graph construction for for loops.

A refactoring in SVN r24088 introduced a bug in for loops.  Loops with
continue in the body and an empty update expression list will lose the
edge from the continue to the loop entry.

R=fschneider@google.com
BUG=11375
Committed: https://code.google.com/p/dart/source/detail?r=24214

[Kevin Millikin (Google), 2013-06-20 09:44:24]

https://codereview.chromium.org/17491003/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (left):

https://codereview.chromium.org/17491003/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:1721: for_increment.Goto(loop_start);
The bug is in the order of side effects here.  If 1720 and 1721 are swapped
(connect the empty increment to the loop start before connecting the continue to
the now-not-empty increment) it will also work.

[Florian Schneider, 2013-06-20 10:15:09]

LGTM.

[Kevin Millikin (Google), 2013-06-20 10:17:42]

Committed patchset #1 manually as r24214 (presubmit successful).

[filemon, 2013-06-20 10:37:01]

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart
File tests/language/for_test.dart (right):

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart#...
tests/language/for_test.dart:45: }
Does this test fail before patching?

I mean, with the bug, the code in attachment (of issue below) always breaks from
loop, and status would be equal 1.

https://code.google.com/p/dart/issues/detail?id=11375

[Kevin Millikin (Google), 2013-06-20 10:48:45]

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart
File tests/language/for_test.dart (right):

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart#...
tests/language/for_test.dart:45: }
On 2013/06/20 10:37:01, filemon wrote:
> Does this test fail before patching?

Yeah, it crashes without the fix.

We should really have exhaustive tests of all for loops:

* with and without initializer
* with and without test
* with and without update
* with and without normal exit from the body
* with and without break
* with and without continue

That's 64 possibilities.

[Ivan Posva, 2013-06-20 17:25:46]

On 2013/06/20 10:48:45, kmillikin wrote:
> https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart
> File tests/language/for_test.dart (right):
> 
>
https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart#...
> tests/language/for_test.dart:45: }
> On 2013/06/20 10:37:01, filemon wrote:
> > Does this test fail before patching?
> 
> Yeah, it crashes without the fix.
> 
> We should really have exhaustive tests of all for loops:
> 
> * with and without initializer
> * with and without test
> * with and without update
> * with and without normal exit from the body
> * with and without break
> * with and without continue
> 
> That's 64 possibilities.

I know this will be a lot of tests, but please add all the variations of for
loops.

-Ivan

[filemon, 2013-06-27 11:44:06]

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart
File tests/language/for_test.dart (right):

https://codereview.chromium.org/17491003/diff/1/tests/language/for_test.dart#...
tests/language/for_test.dart:45: }
Hi, I wrote a simple script to generate loops:
https://gist.github.com/orian/5875825

It needs some adjustment, what do you think?

On 2013/06/20 10:48:45, kmillikin wrote:
> On 2013/06/20 10:37:01, filemon wrote:
> > Does this test fail before patching?
> 
> Yeah, it crashes without the fix.
> 
> We should really have exhaustive tests of all for loops:
> 
> * with and without initializer
> * with and without test
> * with and without update
> * with and without normal exit from the body
> * with and without break
> * with and without continue
> 
> That's 64 possibilities.