From 69fe4ad634d5b839bed1f6a3abd565223d30c447 Mon Sep 17 00:00:00 2001 From: Valeriy Savchenko Date: Mon, 8 Feb 2021 18:47:21 +0300 Subject: [PATCH 1/3] [-Wcompletion-handler] Support checks with builtins It is very common to check callbacks and completion handlers for null. This patch supports such checks using built-in functions: * __builtin_expect * __builtin_expect_with_probablity * __builtin_unpredictable rdar://73455388 Differential Revision: https://reviews.llvm.org/D96268 (cherry picked from commit d1522d349f4d4b960ff7a37303103e95aa535af3) --- clang/lib/Analysis/CalledOnceCheck.cpp | 24 +++++++++ clang/test/SemaObjC/warn-called-once.m | 69 ++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp index 883629a300dcb..92d68d85fbc28 100644 --- a/clang/lib/Analysis/CalledOnceCheck.cpp +++ b/clang/lib/Analysis/CalledOnceCheck.cpp @@ -22,6 +22,7 @@ #include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Analysis/CFG.h" #include "clang/Analysis/FlowSensitive/DataflowWorklist.h" +#include "clang/Basic/Builtins.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LLVM.h" #include "llvm/ADT/BitVector.h" @@ -330,6 +331,29 @@ class DeclRefFinder return Visit(OVE->getSourceExpr()); } + const DeclRefExpr *VisitCallExpr(const CallExpr *CE) { + if (!ShouldRetrieveFromComparisons) + return nullptr; + + // We want to see through some of the boolean builtin functions + // that we are likely to see in conditions. + switch (CE->getBuiltinCallee()) { + case Builtin::BI__builtin_expect: + case Builtin::BI__builtin_expect_with_probability: { + assert(CE->getNumArgs() >= 2); + + const DeclRefExpr *Candidate = Visit(CE->getArg(0)); + return Candidate != nullptr ? Candidate : Visit(CE->getArg(1)); + } + + case Builtin::BI__builtin_unpredictable: + return Visit(CE->getArg(0)); + + default: + return nullptr; + } + } + const DeclRefExpr *VisitExpr(const Expr *E) { // It is a fallback method that gets called whenever the actual type // of the given expression is not covered. diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m index 094f92a49935b..0c11d0e0a15bf 100644 --- a/clang/test/SemaObjC/warn-called-once.m +++ b/clang/test/SemaObjC/warn-called-once.m @@ -4,6 +4,11 @@ #define nil (id)0 #define CALLED_ONCE __attribute__((called_once)) #define NORETURN __attribute__((noreturn)) +#define LIKELY(X) __builtin_expect(!!(X), 1) +#define UNLIKELY(X) __builtin_expect(!!(X), 0) +#define LIKELY_WITH_PROBA(X, P) __builtin_expect_with_probability(!!(X), 1, P) +#define UNLIKELY_WITH_PROBA(X, P) __builtin_expect_with_probability(!!(X), 0, P) +#define UNPRED(X) __builtin_unpredictable((long)(X)) @protocol NSObject @end @@ -547,6 +552,70 @@ int call_with_check_7(int (^callback)(void) CALLED_ONCE) { // no-warning } +void call_with_builtin_check_1(int (^callback)(void) CALLED_ONCE) { + if (LIKELY(callback)) + callback(); + // no-warning +} + +void call_with_builtin_check_2(int (^callback)(void) CALLED_ONCE) { + if (!UNLIKELY(callback)) { + } else { + callback(); + } + // no-warning +} + +void call_with_builtin_check_3(int (^callback)(void) CALLED_ONCE) { + if (__builtin_expect((long)callback, 0L)) { + } else { + callback(); + } + // no-warning +} + +void call_with_builtin_check_4(int (^callback)(void) CALLED_ONCE) { + if (__builtin_expect(0L, (long)callback)) { + } else { + callback(); + } + // no-warning +} + +void call_with_builtin_check_5(int (^callback)(void) CALLED_ONCE) { + if (LIKELY_WITH_PROBA(callback, 0.9)) + callback(); + // no-warning +} + +void call_with_builtin_check_6(int (^callback)(void) CALLED_ONCE) { + if (!UNLIKELY_WITH_PROBA(callback, 0.9)) { + } else { + callback(); + } + // no-warning +} + +void call_with_builtin_check_7(int (^callback)(void) CALLED_ONCE) { + if (UNPRED(callback)) { + } else { + callback(); + } + // no-warning +} + +void call_with_builtin_check_8(int (^callback)(void) CALLED_ONCE) { + if (LIKELY(callback != nil)) + callback(); + // no-warning +} + +void call_with_builtin_check_9(int (^callback)(void) CALLED_ONCE) { + if (!UNLIKELY(callback == NULL)) + callback(); + // no-warning +} + void unreachable_true_branch(void (^callback)(void) CALLED_ONCE) { if (0) { From 8410c68c8385f11d75d8618d3662ec7722042028 Mon Sep 17 00:00:00 2001 From: Valeriy Savchenko Date: Tue, 9 Feb 2021 13:48:24 +0300 Subject: [PATCH 2/3] [-Wcompletion-handler][NFC] Remove unexpected warnings on Windows (cherry picked from commit 2f994d4ee920983cf7624ce2208756e0c7d19007) --- clang/test/SemaObjC/warn-called-once.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m index 0c11d0e0a15bf..1014e17301638 100644 --- a/clang/test/SemaObjC/warn-called-once.m +++ b/clang/test/SemaObjC/warn-called-once.m @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -verify -fsyntax-only -fblocks -fobjc-exceptions -Wcompletion-handler %s +// RUN: %clang_cc1 -verify -fsyntax-only -fblocks -fobjc-exceptions -Wcompletion-handler -Wno-pointer-to-int-cast %s #define NULL (void *)0 #define nil (id)0 From 3335e4946a421bf2cb5a08be8ecaab4a3dcea139 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Tue, 9 Feb 2021 23:21:20 -0800 Subject: [PATCH 3/3] Revert "[analyzer] RetainCountChecker: Add a suppression for OSSymbols." This reverts commit 3500cc8d891bb3825bb3275affe6db8b12f2f695. This old commit was made over a completely false premise. OSSymbols aren't different from other OSObjects and we shouldn't treat them differently for the purposes of static analysis. (cherry picked from commit ddb01010b275c68deb7340ec32e0d1beaa9bbf13) --- clang/lib/Analysis/RetainSummaryManager.cpp | 4 +--- clang/test/Analysis/osobject-retain-release.cpp | 10 ---------- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/clang/lib/Analysis/RetainSummaryManager.cpp b/clang/lib/Analysis/RetainSummaryManager.cpp index 9f45a8efe546f..00bc854a88049 100644 --- a/clang/lib/Analysis/RetainSummaryManager.cpp +++ b/clang/lib/Analysis/RetainSummaryManager.cpp @@ -146,9 +146,7 @@ static bool isSubclass(const Decl *D, } static bool isOSObjectSubclass(const Decl *D) { - // OSSymbols are particular OSObjects that are allocated globally - // and therefore aren't really refcounted, so we ignore them. - return D && isSubclass(D, "OSMetaClassBase") && !isSubclass(D, "OSSymbol"); + return D && isSubclass(D, "OSMetaClassBase"); } static bool isOSObjectDynamicCast(StringRef S) { diff --git a/clang/test/Analysis/osobject-retain-release.cpp b/clang/test/Analysis/osobject-retain-release.cpp index d88349dcd807e..41606a30c39f4 100644 --- a/clang/test/Analysis/osobject-retain-release.cpp +++ b/clang/test/Analysis/osobject-retain-release.cpp @@ -53,9 +53,6 @@ struct MyArray : public OSArray { OSObject *generateObject(OSObject *input) override; }; -// These are never refcounted. -struct OSSymbol : OSObject {}; - struct OtherStruct { static void doNothingToArray(OSArray *array); OtherStruct(OSArray *arr); @@ -757,10 +754,3 @@ void test() { b(0); } } // namespace inherited_constructor_crash - -namespace ossymbol_suppression { -OSSymbol *createSymbol(); -void test() { - OSSymbol *sym = createSymbol(); // no-warning -} -} // namespace ossymbol_suppression