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

lib/compiler/implementation/ssa/builder.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 class Interceptors {
   Compiler compiler;
   Interceptors(Compiler this.compiler);
 
   SourceString mapOperatorToMethodName(Operator op) {
     String name = op.source.stringValue;
@@ skipping 3793 matching lines @@
     work.allowSpeculativeOptimization = false;
     // Save the current locals. The catch block and the finally block
     // must not reuse the existing locals handler. None of the variables
     // that have been defined in the body-block will be used, but for
     // loops we will add (unnecessary) phis that will reference the body
     // variables. This makes it look as if the variables were used
     // in a non-dominated block.
     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);
       exception = new HParameterValue(element);
       add(exception);
       HInstruction oldRethrowableException = rethrowableException;
       rethrowableException = exception;
 
       pushInvokeHelper1(interceptors.getExceptionUnwrapper(), exception);
       HInvokeStatic unwrappedException = pop();
@@ skipping 53 matching lines @@
         } else {
           CatchBlock newBlock = link.head;
           handleIf(node,
                    () { pushCondition(newBlock); },
                    visitThen, visitElse);
         }
       }
 
       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;
     open(exitBlock);
     enterBlock.setBlockFlow(
         new HTryBlockInformation(
           wrapStatementGraph(bodyGraph),
           exception,
@@ skipping 413 matching lines @@
           new HSubGraphBlockInformation(elseBranch.graph));
 
     HBasicBlock conditionStartBlock = conditionBranch.block;
     conditionStartBlock.setBlockFlow(info, joinBlock);
     SubGraph conditionGraph = conditionBranch.graph;
     HIf branch = conditionGraph.end.last;
     assert(branch is HIf);
     branch.blockInformation = conditionStartBlock.blockFlow;
   }
 }