Make dead return statement detection more robust against changes in the CFG.

[klimek]

Summary

This change is a precondition to the proposed change
http://reviews.llvm.org/D3627. I think it's also generally helpful, as it
reduces the number of invariants of the CFG this code relies on.

The idea is to explicitly search for the next return that doesn't have other
paths into it (that is, if the current block is dead, the block containing the
return must be dead, too).

[klimek]

Wordsmithing.

[jordan_rose]

lib/Analysis/ReachableCode.cpp

84

If the block does contain a statement, and it's not a return, then we don't want to keep searching, do we?

[klimek]

lib/Analysis/ReachableCode.cpp

84

Why? As long as the statement doesn't add control flow, the next return statement would still be dead, and we'd want to warn on the dead return instead (at least that's how I understood what we're trying to do here :)

[jordan_rose]

Okay, I think I understand now. If the first dead statement is the first expression in a return statement, and then there's a temporary destructors block, and then the return statement, then we'd still want to treat that as part of the return statement. And that happens right now because we don't optimize out the case with no control flow.

I'm worried, though, that this will catch something in a dead else block and go sailing off the end to look for a return statement, build a parent map, and then not actually find anything (of course). It seems like a fair amount of extra work. Then again, we have already decided to emit a diagnostic at this point, so I guess it's okay.

[klimek]

Just saw that you "accepted" this in phab together with your "so I guess
it's okay" comment ;)

Submitted as r209531.

lib/Analysis/ReachableCode.cpp

if (const DoStmt *DS = dyn_cast<DoStmt>(Term)) {
       const Expr *Cond = DS->getCond()->IgnoreParenCasts();
       return Cond == S && isTrivialExpression(Cond);
     }
   }
   return false;
 }
 
 static bool isDeadReturn(const CFGBlock *B, const Stmt *S) {
-  // Look to see if the block ends with a 'return', and see if 'S'
-  // is a substatement.  The 'return' may not be the last element in
-  // the block because of destructors.
-  for (CFGBlock::const_reverse_iterator I = B->rbegin(), E = B->rend();
+  // Look to see if the current control flow ends with a 'return', and see if
+  // 'S' is a substatement. The 'return' may not be the last element in the
+  // block, or may be in a subsequent block because of destructors.
+  const CFGBlock *Current = B;
+  while (true) {
+    for (CFGBlock::const_reverse_iterator I = Current->rbegin(),
+                                          E = Current->rend();
          I != E; ++I) {
       if (Optional<CFGStmt> CS = I->getAs<CFGStmt>()) {
         if (const ReturnStmt *RS = dyn_cast<ReturnStmt>(CS->getStmt())) {
           if (RS == S)
             return true;
           if (const Expr *RE = RS->getRetValue()) {
             RE = RE->IgnoreParenCasts();
             if (RE == S)
               return true;
-          ParentMap PM(const_cast<Expr*>(RE));
+            ParentMap PM(const_cast<Expr *>(RE));
             // If 'S' is in the ParentMap, it is a subexpression of
-          // the return statement.  Note also that we are restricting
-          // to looking at return statements in the same CFGBlock,
-          // so this will intentionally not catch cases where the
-          // return statement contains nested control-flow.
+            // the return statement.
             return PM.getParent(S);
           }
         }
         break;
       }
     }
+    // Note also that we are restricting the search for the return statement
+    // to stop at control-flow; only part of a return statement may be dead,
+    // without the whole return statement being dead.
+    if (Current->getTerminator().isTemporaryDtorsBranch()) {
+      // Temporary destructors have a predictable control flow, thus we want to
+      // look into the next block for the return statement.
+      // We look into the false branch, as we know the true branch only contains
+      // the call to the destructor.
+      assert(Current->succ_size() == 2);
+      Current = *(Current->succ_begin() + 1);
+    } else if (!Current->getTerminator() && Current->succ_size() == 1) {
+      // If there is only one successor, we're not dealing with outgoing control
+      // flow. Thus, look into the next block.
+      Current = *Current->succ_begin();
+      if (Current->pred_size() > 1) {
+        // If there is more than one predecessor, we're dealing with incoming
+        // control flow - if the return statement is in that block, it might
+        // well be reachable via a different control flow, thus it's not dead.
         return false;
       }
+    } else {
+      // We hit control flow or a dead end. Stop searching.
+      return false;
+    }
+  }
+  llvm_unreachable("Broke out of infinite loop.");
+}
 
 static SourceLocation getTopMostMacro(SourceLocation Loc, SourceManager &SM) {
   assert(Loc.isMacroID());
   SourceLocation Last;
   while (Loc.isMacroID()) {
     Last = Loc;
     Loc = SM.getImmediateMacroCallerLoc(Loc);
   }