[clang] c86dacd - [-Wcalled-once-parameter] Let escapes overwrite MaybeCalled states

[Valeriy Savchenko via cfe-commits]

Author: Valeriy Savchenko
Date: 2021-03-17T11:12:55+03:00
New Revision: c86dacd1a4489572721dec3135506d31da96c679

URL: https://github.com/llvm/llvm-project/commit/c86dacd1a4489572721dec3135506d31da96c679
DIFF: https://github.com/llvm/llvm-project/commit/c86dacd1a4489572721dec3135506d31da96c679.diff

LOG: [-Wcalled-once-parameter] Let escapes overwrite MaybeCalled states

This commit makes escapes symmetrical, meaning that having escape
before and after the branching, where parameter is not called on
one of the paths, will have the same effect.

Differential Revision: https://reviews.llvm.org/D98622

Added: 
    

Modified: 
    clang/lib/Analysis/CalledOnceCheck.cpp
    clang/test/SemaObjC/warn-called-once.m

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp
index 2438c50d7e4e..d24e0b500564 100644
--- a/clang/lib/Analysis/CalledOnceCheck.cpp
+++ b/clang/lib/Analysis/CalledOnceCheck.cpp
@@ -867,16 +867,14 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
     // Let's check if any of the call arguments is a point of interest.
     for (const auto &Argument : llvm::enumerate(Arguments)) {
       if (auto Index = getIndexOfExpression(Argument.value())) {
-        ParameterStatus &CurrentParamStatus = CurrentState.getStatusFor(*Index);
-
         if (shouldBeCalledOnce(CallOrMessage, Argument.index())) {
           // If the corresponding parameter is marked as 'called_once' we should
           // consider it as a call.
           processCallFor(*Index, CallOrMessage);
-        } else if (CurrentParamStatus.getKind() == ParameterStatus::NotCalled) {
+        } else {
           // Otherwise, we mark this parameter as escaped, which can be
           // interpreted both as called or not called depending on the context.
-          CurrentParamStatus = ParameterStatus::Escaped;
+          processEscapeFor(*Index);
         }
         // Otherwise, let's keep the state as it is.
       }
@@ -910,6 +908,16 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
     }
   }
 
+  /// Process escape of the parameter with the given index
+  void processEscapeFor(unsigned Index) {
+    ParameterStatus &CurrentParamStatus = CurrentState.getStatusFor(Index);
+
+    // Escape overrides whatever error we think happened.
+    if (CurrentParamStatus.isErrorStatus()) {
+      CurrentParamStatus = ParameterStatus::Escaped;
+    }
+  }
+
   void findAndReportNotCalledBranches(const CFGBlock *Parent, unsigned Index,
                                       bool IsEscape = false) {
     for (const CFGBlock *Succ : Parent->succs()) {
@@ -1365,11 +1373,7 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
   /// Check given parameter that was discovered to escape.
   void checkEscapee(const ParmVarDecl &Parameter) {
     if (auto Index = getIndex(Parameter)) {
-      ParameterStatus &CurrentParamStatus = CurrentState.getStatusFor(*Index);
-
-      if (CurrentParamStatus.getKind() == ParameterStatus::NotCalled) {
-        CurrentParamStatus = ParameterStatus::Escaped;
-      }
+      processEscapeFor(*Index);
     }
   }
 

diff  --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m
index 3d846deca921..7d0679035238 100644
--- a/clang/test/SemaObjC/warn-called-once.m
+++ b/clang/test/SemaObjC/warn-called-once.m
@@ -1130,4 +1130,32 @@ - (void)test_nil_suppression_3:(int)cond1
   }
 }
 
+- (void)test_escape_before_branch:(int)cond
+                   withCompletion:(void (^)(void))handler {
+  if (cond) {
+    filler();
+  }
+
+  void (^copiedHandler)(void) = ^{
+    handler();
+  };
+
+  if (cond) {
+    // no-warning
+    handler();
+  } else {
+    copiedHandler();
+  }
+}
+
+- (void)test_escape_after_branch:(int)cond
+                  withCompletion:(void (^)(void))handler {
+  if (cond) {
+    // no-warning
+    handler();
+  }
+
+  escape(handler);
+}
+
 @end