Warn when jumping out of a __finally block via continue, break, return, __leave.

Since continue, break, return are much more common than __finally, this tries
to keep the work for continue, break, return O(1).  Sema keeps a stack of active
__finally scopes (to do this, ActOnSEHFinally() is split into
ActOnStartSEHFinally() and ActOnFinishSEHFinally()), and the various jump
statements then check if the current __finally scope (if present) is deeper
than then destination scope of the jump.

The same warning for goto statements is still missing.

This is the moral equivalent of MSVC's C4532.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@231623 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 299844ecb94d3094c8fb057ecb5a7e82997c82f6..226c5ec6ef7c93dde64c6d328f3b9d7eb0633215 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5490,6 +5490,9 @@ def err_mixing_cxx_try_seh_try : Error<
   "cannot use C++ 'try' in the same function as SEH '__try'">;
 def note_conflicting_try_here : Note<
   "conflicting %0 here">;
+def warn_jump_out_of_seh_finally : Warning<
+  "jump out of __finally block has undefined behavior">,
+  InGroup<DiagGroup<"jump-seh-finally">>;
 def warn_non_virtual_dtor : Warning<
   "%0 has virtual functions but non-virtual destructor">,
   InGroup<NonVirtualDtor>, DefaultIgnore;

diff --git a/include/clang/Sema/Scope.h b/include/clang/Sema/Scope.h
index cc6d9cca5b19d17801bd547228bc042ba0d98e8b..238469a2d5d373bb1e10675d4dbfbae5a153f65b 100644 (file)
--- a/include/clang/Sema/Scope.h
+++ b/include/clang/Sema/Scope.h
@@ -416,6 +416,12 @@ public:
   /// \brief Determine whether this scope is a SEH '__except' block.
   bool isSEHExceptScope() const { return getFlags() & Scope::SEHExceptScope; }
 
+  /// \brief Returns if rhs has a higher scope depth than this.
+  ///
+  /// The caller is responsible for calling this only if one of the two scopes
+  /// is an ancestor of the other.
+  bool Contains(const Scope& rhs) const { return Depth < rhs.Depth; }
+
   /// containedInPrototypeScope - Return true if this or a parent scope
   /// is a FunctionPrototypeScope.
   bool containedInPrototypeScope() const;

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 460111f522dfb53a0d3d6266701d3ed81672e382..9879ed96110209d651c0f3dc17cf27a4a362e04a 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -303,6 +303,9 @@ public:
   /// The stack always has at least one element in it.
   SmallVector<MSVtorDispAttr::Mode, 2> VtorDispModeStack;
 
+  /// Stack of active SEH __finally scopes.  Can be empty.
+  SmallVector<Scope*, 2> CurrentSEHFinally;
+
   /// \brief Source location for newly created implicit MSInheritanceAttrs
   SourceLocation ImplicitMSInheritanceAttrLoc;
 
@@ -3293,7 +3296,8 @@ public:
   StmtResult ActOnSEHExceptBlock(SourceLocation Loc,
                                  Expr *FilterExpr,
                                  Stmt *Block);
-  StmtResult ActOnSEHFinallyBlock(SourceLocation Loc, Stmt *Block);
+  void ActOnStartSEHFinallyBlock();
+  StmtResult ActOnFinishSEHFinallyBlock(SourceLocation Loc, Stmt *Block);
   StmtResult ActOnSEHLeaveStmt(SourceLocation Loc, Scope *CurScope);
 
   void DiagnoseReturnInConstructorExceptionHandler(CXXTryStmt *TryBlock);

diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
index e77f07ab030cd1e8d9880b13056025aab0af62ab..9028e4ac2dd8957b69ab21eb4793cd375121a702 100644 (file)
--- a/lib/Parse/ParseStmt.cpp
+++ b/lib/Parse/ParseStmt.cpp
@@ -507,7 +507,7 @@ StmtResult Parser::ParseSEHExceptBlock(SourceLocation ExceptLoc) {
 /// seh-finally-block:
 ///   '__finally' compound-statement
 ///
-StmtResult Parser::ParseSEHFinallyBlock(SourceLocation FinallyBlock) {
+StmtResult Parser::ParseSEHFinallyBlock(SourceLocation FinallyLoc) {
   PoisonIdentifierRAIIObject raii(Ident__abnormal_termination, false),
     raii2(Ident___abnormal_termination, false),
     raii3(Ident_AbnormalTermination, false);
@@ -515,11 +515,14 @@ StmtResult Parser::ParseSEHFinallyBlock(SourceLocation FinallyBlock) {
   if (Tok.isNot(tok::l_brace))
     return StmtError(Diag(Tok, diag::err_expected) << tok::l_brace);
 
+  ParseScope FinallyScope(this, 0);
+  Actions.ActOnStartSEHFinallyBlock();
+
   StmtResult Block(ParseCompoundStatement());
   if(Block.isInvalid())
     return Block;
 
-  return Actions.ActOnSEHFinallyBlock(FinallyBlock,Block.get());
+  return Actions.ActOnFinishSEHFinallyBlock(FinallyLoc, Block.get());
 }
 
 /// Handle __leave

diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index aaf29db702c3979b947f89f710fe6790f8a756c0..11ec4f5321e968d1c297ec8ecf8b1f9a8d61630c 100644 (file)
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -2428,6 +2428,14 @@ Sema::ActOnIndirectGotoStmt(SourceLocation GotoLoc, SourceLocation StarLoc,
   return new (Context) IndirectGotoStmt(GotoLoc, StarLoc, E);
 }
 
+static void CheckJumpOutOfSEHFinally(Sema &S, SourceLocation Loc,
+                                     const Scope &DestScope) {
+  if (!S.CurrentSEHFinally.empty() &&
+      DestScope.Contains(*S.CurrentSEHFinally.back())) {
+    S.Diag(Loc, diag::warn_jump_out_of_seh_finally);
+  }
+}
+
 StmtResult
 Sema::ActOnContinueStmt(SourceLocation ContinueLoc, Scope *CurScope) {
   Scope *S = CurScope->getContinueParent();
@@ -2435,6 +2443,7 @@ Sema::ActOnContinueStmt(SourceLocation ContinueLoc, Scope *CurScope) {
     // C99 6.8.6.2p1: A break shall appear only in or as a loop body.
     return StmtError(Diag(ContinueLoc, diag::err_continue_not_in_loop));
   }
+  CheckJumpOutOfSEHFinally(*this, ContinueLoc, *S);
 
   return new (Context) ContinueStmt(ContinueLoc);
 }
@@ -2449,6 +2458,7 @@ Sema::ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope) {
   if (S->isOpenMPLoopScope())
     return StmtError(Diag(BreakLoc, diag::err_omp_loop_cannot_use_stmt)
                      << "break");
+  CheckJumpOutOfSEHFinally(*this, BreakLoc, *S);
 
   return new (Context) BreakStmt(BreakLoc);
 }
@@ -2908,6 +2918,8 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
     CurScope->setNoNRVO();
   }
 
+  CheckJumpOutOfSEHFinally(*this, ReturnLoc, *CurScope->getFnParent());
+
   return R;
 }
 
@@ -3406,11 +3418,14 @@ Sema::ActOnSEHExceptBlock(SourceLocation Loc,
   return SEHExceptStmt::Create(Context,Loc,FilterExpr,Block);
 }
 
-StmtResult
-Sema::ActOnSEHFinallyBlock(SourceLocation Loc,
-                           Stmt *Block) {
+void Sema::ActOnStartSEHFinallyBlock() {
+  CurrentSEHFinally.push_back(CurScope);
+}
+
+StmtResult Sema::ActOnFinishSEHFinallyBlock(SourceLocation Loc, Stmt *Block) {
   assert(Block);
-  return SEHFinallyStmt::Create(Context,Loc,Block);
+  CurrentSEHFinally.pop_back();
+  return SEHFinallyStmt::Create(Context, Loc, Block);
 }
 
 StmtResult
@@ -3420,6 +3435,7 @@ Sema::ActOnSEHLeaveStmt(SourceLocation Loc, Scope *CurScope) {
     SEHTryParent = SEHTryParent->getParent();
   if (!SEHTryParent)
     return StmtError(Diag(Loc, diag::err_ms___leave_not_in___try));
+  CheckJumpOutOfSEHFinally(*this, Loc, *SEHTryParent);
 
   return new (Context) SEHLeaveStmt(Loc);
 }

diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h
index e1d0d18dde47093bff11564e5624b5307aa73678..351dacd256e2d236551653c291a49c6f79020ccc 100644 (file)
--- a/lib/Sema/TreeTransform.h
+++ b/lib/Sema/TreeTransform.h
@@ -1702,7 +1702,7 @@ public:
   }
 
   StmtResult RebuildSEHFinallyStmt(SourceLocation Loc, Stmt *Block) {
-    return getSema().ActOnSEHFinallyBlock(Loc, Block);
+    return SEHFinallyStmt::Create(getSema().getASTContext(), Loc, Block);
   }
 
   /// \brief Build a new predefined expression.

diff --git a/test/Sema/__try.c b/test/Sema/__try.c
index 0e5de2018db0a0d8f215d9a1603d4e21e855daff..925dbcc55ef1fe99197b261af067c007813ca99c 100644 (file)
--- a/test/Sema/__try.c
+++ b/test/Sema/__try.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fborland-extensions -DBORLAND -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fborland-extensions -DBORLAND -fsyntax-only -verify -fblocks %s
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify -fblocks %s
 
 #define JOIN2(x,y) x ## y
 #define JOIN(x,y) JOIN2(x,y)
@@ -207,3 +207,77 @@ void test_seh_leave_stmt() {
   }
   __leave; // expected-error{{'__leave' statement not in __try block}}
 }
+
+void test_jump_out_of___finally() {
+  while(1) {
+    __try {
+    } __finally {
+      continue; // expected-warning{{jump out of __finally block has undefined behavior}}
+    }
+  }
+  __try {
+  } __finally {
+    while (1) {
+      continue;
+    }
+  }
+
+  // Check that a deep __finally containing a block with a shallow continue
+  // doesn't trigger the warning.
+  while(1) {{{{
+    __try {
+    } __finally {
+      ^{
+        while(1)
+          continue;
+      }();
+    }
+  }}}}
+
+  while(1) {
+    __try {
+    } __finally {
+      break; // expected-warning{{jump out of __finally block has undefined behavior}}
+    }
+  }
+  switch(1) {
+  case 1:
+    __try {
+    } __finally {
+      break; // expected-warning{{jump out of __finally block has undefined behavior}}
+    }
+  }
+  __try {
+  } __finally {
+    while (1) {
+      break;
+    }
+  }
+
+  __try {
+    __try {
+    } __finally {
+      __leave; // expected-warning{{jump out of __finally block has undefined behavior}}
+    }
+  } __finally {
+  }
+  __try {
+  } __finally {
+    __try {
+      __leave;
+    } __finally {
+    }
+  }
+
+  __try {
+  } __finally {
+    return; // expected-warning{{jump out of __finally block has undefined behavior}}
+  }
+
+  __try {
+  } __finally {
+    ^{
+      return;
+    }();
+  }
+}