Fix issue 5517 by setting the successors the right way in a try/catch.

lib/compiler/implementation/ssa/builder.dart

Index: lib/compiler/implementation/ssa/builder.dart
===================================================================
--- lib/compiler/implementation/ssa/builder.dart	(revision 12963)
+++ lib/compiler/implementation/ssa/builder.dart	(working copy)
@@ -3811,22 +3811,27 @@
     LocalsHandler savedLocals = new LocalsHandler.from(localsHandler);
     HBasicBlock enterBlock = openNewBlock();
     HTry tryInstruction = new HTry();
-    List<HBasicBlock> blocks = <HBasicBlock>[];
-    blocks.add(close(tryInstruction));
+    close(tryInstruction);
 
-    HBasicBlock tryBody = graph.addNewBlock();
-    enterBlock.addSuccessor(tryBody);
-    open(tryBody);
+    HBasicBlock startBodyBlock;

[kasperl, 2012/09/28 06:20:06]

startTryBlock?
endTryBlock?

[ngeoffray, 2012/09/28 08:20:51]

Done.

+    HBasicBlock endBodyBlock;
+    HBasicBlock startCatchBlock;
+    HBasicBlock endCatchBlock;
+    HBasicBlock startFinallyBlock;
+    HBasicBlock endFinallyBlock;
+
+    startBodyBlock = graph.addNewBlock();
+    open(startBodyBlock);
     visit(node.tryBlock);
-    if (!isAborted()) blocks.add(close(new HGoto()));
-    SubGraph bodyGraph = new SubGraph(tryBody, lastOpenedBlock);
+    if (!isAborted()) endBodyBlock = close(new HGoto());
+    SubGraph bodyGraph = new SubGraph(startBodyBlock, lastOpenedBlock);
     SubGraph catchGraph = null;
     HParameterValue exception = null;
+
     if (!node.catchBlocks.isEmpty()) {
       localsHandler = new LocalsHandler.from(savedLocals);
-      HBasicBlock block = graph.addNewBlock();
-      enterBlock.addSuccessor(block);
-      open(block);
+      startCatchBlock = graph.addNewBlock();
+      open(startCatchBlock);
       // Note that the name of this element is irrelevant.
       Element element = new Element(
           const SourceString('exception'), ElementKind.PARAMETER, work.element);
@@ -3900,31 +3905,65 @@
 
       CatchBlock firstBlock = link.head;
       handleIf(node, () { pushCondition(firstBlock); }, visitThen, visitElse);
-      if (!isAborted()) blocks.add(close(new HGoto()));
+      if (!isAborted()) endCatchBlock = close(new HGoto());
 
       rethrowableException = oldRethrowableException;
-      tryInstruction.catchBlock = block;
-      catchGraph = new SubGraph(block, lastOpenedBlock);
+      tryInstruction.catchBlock = startCatchBlock;
+      catchGraph = new SubGraph(startCatchBlock, lastOpenedBlock);
     }
 
     SubGraph finallyGraph = null;
     if (node.finallyBlock != null) {
       localsHandler = new LocalsHandler.from(savedLocals);
-      HBasicBlock finallyBlock = graph.addNewBlock();
-      enterBlock.addSuccessor(finallyBlock);
-      open(finallyBlock);
+      startFinallyBlock = graph.addNewBlock();
+      open(startFinallyBlock);
       visit(node.finallyBlock);
-      if (!isAborted()) blocks.add(close(new HGoto()));
-      tryInstruction.finallyBlock = finallyBlock;
-      finallyGraph = new SubGraph(finallyBlock, lastOpenedBlock);
+      if (!isAborted()) endFinallyBlock = close(new HGoto());
+      tryInstruction.finallyBlock = startFinallyBlock;
+      finallyGraph = new SubGraph(startFinallyBlock, lastOpenedBlock);
     }
 
     HBasicBlock exitBlock = graph.addNewBlock();
 
-    for (HBasicBlock block in blocks) {
-      block.addSuccessor(exitBlock);
+    // Setup all successors. The entry block that contains the [HTry]
+    // has 1) the body, 2) the catch, 3) the finally, and 4) the exit
+    // blocks as successors.
+    enterBlock.addSuccessor(startBodyBlock);
+    if (startCatchBlock != null) {
+      enterBlock.addSuccessor(startCatchBlock);

[kasperl, 2012/09/28 06:20:06]

Maybe add a helper for adding an optional successor (adding it if it isn't
null). Use it in these two cases.

[ngeoffray, 2012/09/28 08:20:51]

Done.

     }
+    if (startFinallyBlock != null) {
+      enterBlock.addSuccessor(startFinallyBlock);
+    }
+    enterBlock.addSuccessor(exitBlock);
 
+    // The body has either the catch, the finally, or the exit block
+    // as successor.
+    if (endBodyBlock != null) {
+      if (startCatchBlock != null) {

[kasperl, 2012/09/28 06:20:06]

If you let the addOptionalSuccessor helper return a bool that indicates whether
or not you added the successor you should be able to use it here. Something
like:

   if (!endBodyBlock.addOptionalSuccessor(startCatchBlock) &&
       !endBodyBlock.addOptionalSuccessor(startFinallBlock)) {
     endBodyBlock.addSuccessor(exitBlock);
   }

Not sure it's that much better but at least it's an option.

[ngeoffray, 2012/09/28 08:20:51]

Soren made a good point that I actually did not need the last if. I changed the
code to use a conditional expression instead.

+        endBodyBlock.addSuccessor(startCatchBlock);
+      } else if (startFinallyBlock != null) {
+        endBodyBlock.addSuccessor(startFinallyBlock);
+      } else {

[Søren Gjesse, 2012/09/27 15:35:28]

Do you ever get here? You will always have at least catch or finally.

[ngeoffray, 2012/09/28 08:20:51]

Good catch. Done.

+        endBodyBlock.addSuccessor(exitBlock);
+      }
+    }
+
+    // The catch block has either the finally or the exit block as
+    // successor.
+    if (endCatchBlock != null) {
+      if (startFinallyBlock != null) {
+        endCatchBlock.addSuccessor(startFinallyBlock);
+      } else {
+        endCatchBlock.addSuccessor(exitBlock);
+      }
+    }
+
+    // The finally block has the exit block as successor.
+    if (endFinallyBlock != null) {
+      endFinallyBlock.addSuccessor(exitBlock);
+    }
+
     // Use the locals handler not altered by the catch and finally
     // blocks.
     localsHandler = savedLocals;