[PATCH] D98694: [-Wcalled-once-parameter] Fix false positives for cleanup attr

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4a7afc9a8843: [-Wcalled-once-parameter] Fix false positives for cleanup attr (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98694/new/

https://reviews.llvm.org/D98694

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


Index: clang/test/SemaObjC/warn-called-once.m
===================================================================
--- clang/test/SemaObjC/warn-called-once.m
+++ clang/test/SemaObjC/warn-called-once.m
@@ -1193,4 +1193,46 @@
   escape(handler);
 }
 
+// rdar://74441906
+typedef void (^DeferredBlock)(void);
+static inline void DefferedCallback(DeferredBlock *inBlock) { (*inBlock)(); }
+#define _DEFERCONCAT(a, b) a##b
+#define _DEFERNAME(a) _DEFERCONCAT(__DeferredVar_, a)
+#define DEFER __extension__ __attribute__((cleanup(DefferedCallback), unused)) \
+                  DeferredBlock _DEFERNAME(__COUNTER__) = ^
+
+- (void)test_cleanup_1:(int)cond
+        withCompletion:(void (^)(void))handler {
+  int error = 0;
+  DEFER {
+    if (error)
+      handler();
+  };
+
+  if (cond) {
+    error = 1;
+  } else {
+    // no-warning
+    handler();
+  }
+}
+
+- (void)test_cleanup_2:(int)cond
+        withCompletion:(void (^)(void))handler {
+  int error = 0;
+  DEFER {
+    if (error)
+      handler();
+  };
+
+  if (cond) {
+    error = 1;
+  } else {
+    handler(); // expected-note{{previous call is here}}
+  }
+
+  // We still can warn about double call even in this case.
+  handler(); // expected-warning{{completion handler is called twice}}
+}
+
 @end
Index: clang/lib/Analysis/CalledOnceCheck.cpp
===================================================================
--- clang/lib/Analysis/CalledOnceCheck.cpp
+++ clang/lib/Analysis/CalledOnceCheck.cpp
@@ -812,8 +812,12 @@
       }
     }
 
-    // Early exit if we don't have parameters for extra analysis.
-    if (NotCalledOnEveryPath.none() && NotUsedOnEveryPath.none())
+    // Early exit if we don't have parameters for extra analysis...
+    if (NotCalledOnEveryPath.none() && NotUsedOnEveryPath.none() &&
+        // ... or if we've seen variables with cleanup functions.
+        // We can't reason that we've seen every path in this case,
+        // and thus abandon reporting any warnings that imply that.
+        !FunctionHasCleanupVars)
       return;
 
     // We are looking for a pair of blocks A, B so that the following is true:
@@ -1601,6 +1605,10 @@
         if (Var->getInit()) {
           checkEscapee(Var->getInit());
         }
+
+        if (Var->hasAttr<CleanupAttr>()) {
+          FunctionHasCleanupVars = true;
+        }
       }
     }
   }
@@ -1669,6 +1677,13 @@
   // around.
   bool SuppressOnConventionalErrorPaths = false;
 
+  // The user can annotate variable declarations with cleanup functions, which
+  // essentially imposes a custom destructor logic on that variable.
+  // It is possible to use it, however, to call tracked parameters on all exits
+  // from the function.  For this reason, we track the fact that the function
+  // actually has these.
+  bool FunctionHasCleanupVars = false;
+
   State CurrentState;
   ParamSizedVector<const ParmVarDecl *> TrackedParams;
   CFGSizedVector<State> States;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D98694.331492.patch
Type: text/x-patch
Size: 2920 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210318/caca4f7e/attachment.bin>