[clang] [alpha.webkit.UncountedCallArgsChecker] Use canonical type (PR #109393)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/109393 This PR fixes a bug in UncountedCallArgsChecker that calling a function with a member variable which is Ref/RefPtr is erroneously treated as safe by canoniclizing the type before checking whether it's ref counted or not. >From 7b74a32cd93f7812ec48b60ffcf379a91c7bdc6c Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 20 Sep 2024 02:02:41 -0700 Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Use canonical type This PR fixes a bug in UncountedCallArgsChecker that calling a function with a member variable which is Ref/RefPtr is erroneously treated as safe by canoniclizing the type before checking whether it's ref counted or not. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- .../WebKit/UncountedCallArgsChecker.cpp | 9 --- .../WebKit/uncounted-obj-const-v-muable.cpp | 27 +++ 3 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 9da3e54e454317..54c99c3c1b37f9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -155,7 +155,7 @@ std::optional isUncounted(const QualType T) { std::optional isUncounted(const CXXRecordDecl* Class) { // Keep isRefCounted first as it's cheaper. - if (isRefCounted(Class)) + if (!Class || isRefCounted(Class)) return false; std::optional IsRefCountable = isRefCountable(Class); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 81c2434ce64775..31e9b3c4b9d412 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -86,7 +86,7 @@ class UncountedCallArgsChecker return; } auto *E = MemberCallExpr->getImplicitObjectArgument(); -QualType ArgType = MemberCallExpr->getObjectType(); +QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType(); std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) reportBugOnThis(E); @@ -102,12 +102,13 @@ class UncountedCallArgsChecker // if ((*P)->hasAttr()) // continue; -const auto *ArgType = (*P)->getType().getTypePtrOrNull(); -if (!ArgType) +QualType ArgType = (*P)->getType().getCanonicalType(); +const auto *TypePtr = ArgType.getTypePtrOrNull(); +if (!TypePtr) continue; // FIXME? Should we bail? // FIXME: more complex types (arrays, references to raw pointers, etc) -std::optional IsUncounted = isUncountedPtr(ArgType); +std::optional IsUncounted = isUncountedPtr(TypePtr); if (!IsUncounted || !(*IsUncounted)) continue; diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp new file mode 100644 index 00..2721cd8474e1b4 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp @@ -0,0 +1,27 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s + +#include "mock-types.h" + +class Object { +public: +void ref() const; +void deref() const; + +bool constFunc() const; +void mutableFunc(); +}; + +class Caller { + void someFunction(); + void otherFunction(); +private: +RefPtr m_obj; +}; + +void Caller::someFunction() +{ +m_obj->constFunc(); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} +m_obj->mutableFunc(); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] WebKit Checkers should set DeclWithIssue. (PR #109389)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/109389 >From b8f95b5b809cbeb8199de6cd24e18a605189f722 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 19 Sep 2024 23:41:10 -0700 Subject: [PATCH 1/3] WebKit Checkers should set DeclWithIssue. Set DeclWithIssue in alpha.webkit.UncountedCallArgsChecker and alpha.webkit.UncountedLocalVarsChecker. --- .../WebKit/UncountedCallArgsChecker.cpp | 29 ++- .../WebKit/UncountedLocalVarsChecker.cpp | 21 ++ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 81c2434ce64775..410e78c5418ee3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -18,6 +18,8 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/Support/SaveAndRestore.h" #include using namespace clang; @@ -44,7 +46,11 @@ class UncountedCallArgsChecker // visit template instantiations or lambda classes. We // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { + using Base = RecursiveASTVisitor; + const UncountedCallArgsChecker *Checker; + Decl *DeclWithIssue { nullptr }; + explicit LocalVisitor(const UncountedCallArgsChecker *Checker) : Checker(Checker) { assert(Checker); @@ -56,12 +62,16 @@ class UncountedCallArgsChecker bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) { if (isRefType(safeGetName(Decl))) return true; -return RecursiveASTVisitor::TraverseClassTemplateDecl( -Decl); +return Base::TraverseClassTemplateDecl(Decl); + } + + bool TraverseDecl(Decl *D) { +llvm::SaveAndRestore SavedDecl(DeclWithIssue, D); +return Base::TraverseDecl(D); } bool VisitCallExpr(const CallExpr *CE) { -Checker->visitCallExpr(CE); +Checker->visitCallExpr(CE, DeclWithIssue); return true; } }; @@ -70,7 +80,7 @@ class UncountedCallArgsChecker visitor.TraverseDecl(const_cast(TUD)); } - void visitCallExpr(const CallExpr *CE) const { + void visitCallExpr(const CallExpr *CE, const Decl *D) const { if (shouldSkipCall(CE)) return; @@ -89,7 +99,7 @@ class UncountedCallArgsChecker QualType ArgType = MemberCallExpr->getObjectType(); std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) - reportBugOnThis(E); + reportBugOnThis(E, D); } for (auto P = F->param_begin(); @@ -119,7 +129,7 @@ class UncountedCallArgsChecker if (isPtrOriginSafe(Arg)) continue; -reportBug(Arg, *P); +reportBug(Arg, *P, D); } } } @@ -240,7 +250,8 @@ class UncountedCallArgsChecker ClsName.ends_with("String")); } - void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const { + void reportBug(const Expr *CallArg, const ParmVarDecl *Param, + const Decl *DeclWithIssue) const { assert(CallArg); SmallString<100> Buf; @@ -261,10 +272,11 @@ class UncountedCallArgsChecker PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); auto Report = std::make_unique(Bug, Os.str(), BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } - void reportBugOnThis(const Expr *CallArg) const { + void reportBugOnThis(const Expr *CallArg, const Decl *DeclWithIssue) const { assert(CallArg); const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin(); @@ -274,6 +286,7 @@ class UncountedCallArgsChecker Bug, "Call argument for 'this' parameter is uncounted and unsafe.", BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } }; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 274da0baf2ce5c..30f10d7e9f91e7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -121,6 +121,7 @@ class UncountedLocalVarsChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { const UncountedLocalVarsChecker *Checker; + Decl *DeclWithIssue { nu
[clang] WebKit Checkers should set DeclWithIssue. (PR #109389)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/109389 Set DeclWithIssue in alpha.webkit.UncountedCallArgsChecker and alpha.webkit.UncountedLocalVarsChecker. >From b8f95b5b809cbeb8199de6cd24e18a605189f722 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 19 Sep 2024 23:41:10 -0700 Subject: [PATCH] WebKit Checkers should set DeclWithIssue. Set DeclWithIssue in alpha.webkit.UncountedCallArgsChecker and alpha.webkit.UncountedLocalVarsChecker. --- .../WebKit/UncountedCallArgsChecker.cpp | 29 ++- .../WebKit/UncountedLocalVarsChecker.cpp | 21 ++ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 81c2434ce64775..410e78c5418ee3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -18,6 +18,8 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/Support/SaveAndRestore.h" #include using namespace clang; @@ -44,7 +46,11 @@ class UncountedCallArgsChecker // visit template instantiations or lambda classes. We // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { + using Base = RecursiveASTVisitor; + const UncountedCallArgsChecker *Checker; + Decl *DeclWithIssue { nullptr }; + explicit LocalVisitor(const UncountedCallArgsChecker *Checker) : Checker(Checker) { assert(Checker); @@ -56,12 +62,16 @@ class UncountedCallArgsChecker bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) { if (isRefType(safeGetName(Decl))) return true; -return RecursiveASTVisitor::TraverseClassTemplateDecl( -Decl); +return Base::TraverseClassTemplateDecl(Decl); + } + + bool TraverseDecl(Decl *D) { +llvm::SaveAndRestore SavedDecl(DeclWithIssue, D); +return Base::TraverseDecl(D); } bool VisitCallExpr(const CallExpr *CE) { -Checker->visitCallExpr(CE); +Checker->visitCallExpr(CE, DeclWithIssue); return true; } }; @@ -70,7 +80,7 @@ class UncountedCallArgsChecker visitor.TraverseDecl(const_cast(TUD)); } - void visitCallExpr(const CallExpr *CE) const { + void visitCallExpr(const CallExpr *CE, const Decl *D) const { if (shouldSkipCall(CE)) return; @@ -89,7 +99,7 @@ class UncountedCallArgsChecker QualType ArgType = MemberCallExpr->getObjectType(); std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) - reportBugOnThis(E); + reportBugOnThis(E, D); } for (auto P = F->param_begin(); @@ -119,7 +129,7 @@ class UncountedCallArgsChecker if (isPtrOriginSafe(Arg)) continue; -reportBug(Arg, *P); +reportBug(Arg, *P, D); } } } @@ -240,7 +250,8 @@ class UncountedCallArgsChecker ClsName.ends_with("String")); } - void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const { + void reportBug(const Expr *CallArg, const ParmVarDecl *Param, + const Decl *DeclWithIssue) const { assert(CallArg); SmallString<100> Buf; @@ -261,10 +272,11 @@ class UncountedCallArgsChecker PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); auto Report = std::make_unique(Bug, Os.str(), BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } - void reportBugOnThis(const Expr *CallArg) const { + void reportBugOnThis(const Expr *CallArg, const Decl *DeclWithIssue) const { assert(CallArg); const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin(); @@ -274,6 +286,7 @@ class UncountedCallArgsChecker Bug, "Call argument for 'this' parameter is uncounted and unsafe.", BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } }; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 274da0baf2ce5c..30f10d7e9f91e7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -121,6 +121,7 @@ class UncountedLocalVarsChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public R
[clang] WebKit Checkers should set DeclWithIssue. (PR #109389)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/109389 >From b8f95b5b809cbeb8199de6cd24e18a605189f722 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 19 Sep 2024 23:41:10 -0700 Subject: [PATCH 1/2] WebKit Checkers should set DeclWithIssue. Set DeclWithIssue in alpha.webkit.UncountedCallArgsChecker and alpha.webkit.UncountedLocalVarsChecker. --- .../WebKit/UncountedCallArgsChecker.cpp | 29 ++- .../WebKit/UncountedLocalVarsChecker.cpp | 21 ++ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 81c2434ce64775..410e78c5418ee3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -18,6 +18,8 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/Support/SaveAndRestore.h" #include using namespace clang; @@ -44,7 +46,11 @@ class UncountedCallArgsChecker // visit template instantiations or lambda classes. We // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { + using Base = RecursiveASTVisitor; + const UncountedCallArgsChecker *Checker; + Decl *DeclWithIssue { nullptr }; + explicit LocalVisitor(const UncountedCallArgsChecker *Checker) : Checker(Checker) { assert(Checker); @@ -56,12 +62,16 @@ class UncountedCallArgsChecker bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) { if (isRefType(safeGetName(Decl))) return true; -return RecursiveASTVisitor::TraverseClassTemplateDecl( -Decl); +return Base::TraverseClassTemplateDecl(Decl); + } + + bool TraverseDecl(Decl *D) { +llvm::SaveAndRestore SavedDecl(DeclWithIssue, D); +return Base::TraverseDecl(D); } bool VisitCallExpr(const CallExpr *CE) { -Checker->visitCallExpr(CE); +Checker->visitCallExpr(CE, DeclWithIssue); return true; } }; @@ -70,7 +80,7 @@ class UncountedCallArgsChecker visitor.TraverseDecl(const_cast(TUD)); } - void visitCallExpr(const CallExpr *CE) const { + void visitCallExpr(const CallExpr *CE, const Decl *D) const { if (shouldSkipCall(CE)) return; @@ -89,7 +99,7 @@ class UncountedCallArgsChecker QualType ArgType = MemberCallExpr->getObjectType(); std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) - reportBugOnThis(E); + reportBugOnThis(E, D); } for (auto P = F->param_begin(); @@ -119,7 +129,7 @@ class UncountedCallArgsChecker if (isPtrOriginSafe(Arg)) continue; -reportBug(Arg, *P); +reportBug(Arg, *P, D); } } } @@ -240,7 +250,8 @@ class UncountedCallArgsChecker ClsName.ends_with("String")); } - void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const { + void reportBug(const Expr *CallArg, const ParmVarDecl *Param, + const Decl *DeclWithIssue) const { assert(CallArg); SmallString<100> Buf; @@ -261,10 +272,11 @@ class UncountedCallArgsChecker PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); auto Report = std::make_unique(Bug, Os.str(), BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } - void reportBugOnThis(const Expr *CallArg) const { + void reportBugOnThis(const Expr *CallArg, const Decl *DeclWithIssue) const { assert(CallArg); const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin(); @@ -274,6 +286,7 @@ class UncountedCallArgsChecker Bug, "Call argument for 'this' parameter is uncounted and unsafe.", BSLoc); Report->addRange(CallArg->getSourceRange()); +Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } }; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 274da0baf2ce5c..30f10d7e9f91e7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -121,6 +121,7 @@ class UncountedLocalVarsChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { const UncountedLocalVarsChecker *Checker; + Decl *DeclWithIssue { nu
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/108656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108656 >From e86f101c5cd87db597c5fc8ab017b20adba98284 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 13 Sep 2024 15:19:45 -0700 Subject: [PATCH 1/2] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings Improve the fix in 203a2ca8cd6af505e11a38aebceeaf864271042c by allowing variable references and more ignoring of parentheses. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 19 ++- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 18 ++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index ecba5f9aa23ee3..5ea19233bb65c2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -84,13 +84,22 @@ class DerefFuncDeleteExprVisitor E = E->IgnoreParenCasts(); if (auto *TempE = dyn_cast(E)) E = TempE->getSubExpr(); +E = E->IgnoreParenCasts(); +if (auto *Ref = dyn_cast(E)) { + if (auto *Decl = Ref->getDecl()) { +if (auto *VD = dyn_cast(Decl)) + return VisitLabmdaArgument(VD->getInit()); + } + return false; +} +if (auto *Lambda = dyn_cast(E)) { + if (VisitBody(Lambda->getBody())) +return true; +} if (auto *ConstructE = dyn_cast(E)) { for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { -auto *Arg = ConstructE->getArg(i); -if (auto *Lambda = dyn_cast(Arg)) { - if (VisitBody(Lambda->getBody())) -return true; -} +if (VisitLabmdaArgument(ConstructE->getArg(i))) + return true; } } return false; diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp index 01527addb52992..33c60ea8ca64d1 100644 --- a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -119,6 +119,11 @@ template ensureOnMainThread([this] { delete static_cast(this); }); +} else if constexpr (destructionThread == DestructionThread::MainRunLoop) { +auto deleteThis = [this] { +delete static_cast(this); +}; +ensureOnMainThread(deleteThis); } } @@ -230,3 +235,16 @@ class FancyRefCountedClass4 final : public BadNestedThreadSafeRefCounted { +public: +static Ref create() +{ +return adoptRef(*new FancyRefCountedClass5()); +} + +virtual ~FancyRefCountedClass5(); + +private: +FancyRefCountedClass5(); +}; >From 1b2c46b0ff21f3a1d87e1610452b73926dd36fdc Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 17 Sep 2024 19:13:31 -0700 Subject: [PATCH 2/2] Address review comments. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp| 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 5ea19233bb65c2..e80246f49a3100 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -72,7 +72,7 @@ class DerefFuncDeleteExprVisitor if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { for (unsigned i = 0; i < CE->getNumArgs(); ++i) { auto *Arg = CE->getArg(i); - if (VisitLabmdaArgument(Arg)) + if (VisitLambdaArgument(Arg)) return true; } } @@ -80,16 +80,14 @@ class DerefFuncDeleteExprVisitor return false; } - bool VisitLabmdaArgument(const Expr *E) { + bool VisitLambdaArgument(const Expr *E) { E = E->IgnoreParenCasts(); if (auto *TempE = dyn_cast(E)) E = TempE->getSubExpr(); E = E->IgnoreParenCasts(); if (auto *Ref = dyn_cast(E)) { - if (auto *Decl = Ref->getDecl()) { -if (auto *VD = dyn_cast(Decl)) - return VisitLabmdaArgument(VD->getInit()); - } + if (auto *VD = dyn_cast_or_null(Ref->getDecl())) +return VisitLambdaArgument(VD->getInit()); return false; } if (auto *Lambda = dyn_cast(E)) { @@ -98,7 +96,7 @@ class DerefFuncDeleteExprVisitor } if (auto *ConstructE = dyn_cast(E)) { for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { -if (VisitLabmdaArgument(ConstructE->getArg(i))) +if (VisitLambdaArgument(ConstructE->getArg(i))) return true; } } __
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
@@ -119,6 +119,11 @@ template ensureOnMainThread([this] { delete static_cast(this); }); +} else if constexpr (destructionThread == DestructionThread::MainRunLoop) { +auto deleteThis = [this] { rniwa wrote: I don't think so. At least we don't do that in WebKit for now. https://github.com/llvm/llvm-project/pull/108656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
@@ -84,13 +84,22 @@ class DerefFuncDeleteExprVisitor E = E->IgnoreParenCasts(); if (auto *TempE = dyn_cast(E)) E = TempE->getSubExpr(); +E = E->IgnoreParenCasts(); +if (auto *Ref = dyn_cast(E)) { + if (auto *Decl = Ref->getDecl()) { +if (auto *VD = dyn_cast(Decl)) rniwa wrote: Oh, neat. Fixed. https://github.com/llvm/llvm-project/pull/108656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
@@ -84,13 +84,22 @@ class DerefFuncDeleteExprVisitor E = E->IgnoreParenCasts(); if (auto *TempE = dyn_cast(E)) E = TempE->getSubExpr(); +E = E->IgnoreParenCasts(); +if (auto *Ref = dyn_cast(E)) { + if (auto *Decl = Ref->getDecl()) { +if (auto *VD = dyn_cast(Decl)) + return VisitLabmdaArgument(VD->getInit()); rniwa wrote: Oops, fixed. https://github.com/llvm/llvm-project/pull/108656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access (PR #108669)
rniwa wrote: > I'm also somewhat terrified of returning C++ objects from ObjC methods by > value, ever since I learned that when you call an ObjC method on a nil it > returns a _zero-initialized_ object without calling a constructor on it. Yikes. I didn't know that! https://github.com/llvm/llvm-project/pull/108669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access (PR #108669)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/108669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access (PR #108669)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108669 >From 241fde13679b85e63126318520cd3f19029f49ad Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 13 Sep 2024 19:01:57 -0700 Subject: [PATCH 1/2] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe. --- .../Checkers/WebKit/ASTUtils.cpp | 15 - .../Checkers/WebKit/PtrTypesSemantics.cpp | 5 ++--- .../Checkers/WebKit/PtrTypesSemantics.h | 4 ++-- .../Checkers/WebKit/uncounted-obj-arg.mm | 22 +++ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index be07cf51eefb3d..394cb26f03cf99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -12,6 +12,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/ExprObjC.h" #include namespace clang { @@ -35,6 +36,12 @@ bool tryToFindPtrOrigin( break; } } +if (auto *POE = dyn_cast(E)) { + if (auto *RF = POE->getResultExpr()) { +E = RF; +continue; + } +} if (auto *tempExpr = dyn_cast(E)) { E = tempExpr->getSubExpr(); continue; @@ -88,7 +95,7 @@ bool tryToFindPtrOrigin( continue; } -if (isReturnValueRefCounted(callee)) +if (isRefType(callee->getReturnType())) return callback(E, true); if (isSingleton(callee)) @@ -100,6 +107,12 @@ bool tryToFindPtrOrigin( } } } +if (auto *ObjCMsgExpr = dyn_cast(E)) { + if (auto *Method = ObjCMsgExpr->getMethodDecl()) { +if (isRefType(Method->getReturnType())) + return callback(E, true); + } +} if (auto *unaryOp = dyn_cast(E)) { // FIXME: Currently accepts ANY unary operator. Is it OK? E = unaryOp->getSubExpr(); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..9da3e54e454317 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -123,9 +123,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) { || FunctionName == "Identifier"; } -bool isReturnValueRefCounted(const clang::FunctionDecl *F) { - assert(F); - QualType type = F->getReturnType(); +bool isRefType(const clang::QualType T) { + QualType type = T; while (!type.isNull()) { if (auto *elaboratedT = type->getAs()) { type = elaboratedT->desugar(); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 2932e62ad06e4b..e2d0342bebd52c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -62,8 +62,8 @@ bool isRefType(const std::string &Name); /// false if not. bool isCtorOfRefCounted(const clang::FunctionDecl *F); -/// \returns true if \p F returns a ref-counted object, false if not. -bool isReturnValueRefCounted(const clang::FunctionDecl *F); +/// \returns true if \p T is RefPtr, Ref, or its variant, false if not. +bool isRefType(const clang::QualType T); /// \returns true if \p M is getter of a ref-counted class, false if not. std::optional isGetterOfRefCounted(const clang::CXXMethodDecl* Method); diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm index db0c5b19eec5bb..1d81a8851ece0c 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm @@ -11,6 +11,7 @@ @interface Foo : NSObject - (void)execute; - (RefPtr)_protectedRefCountable; +- (Ref)_protectedRefCountable2; @end @implementation Foo @@ -23,4 +24,25 @@ - (void)execute { return _countable; } +- (Ref)_protectedRefCountable2 { + return *_countable; +} + @end + +class RefCountedObject { +public: + void ref() const; + void deref() const; + Ref copy() const; +}; + +@interface WrapperObj : NSObject + +- (Ref)_protectedWebExtensionControllerConfiguration; + +@end + +static void foo(WrapperObj *configuration) { + configuration._protectedWebExtensionControllerConfiguration->copy(); +} >From 89565cebeda804a34359f75bb4fd64154aa429ac Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 13 Sep 2024 19:04:48 -0700 Subject: [PATCH 2/2] Remove the superflous method. --- clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm | 5 - 1 file changed, 5 deletions(-) diff --git a/clang/test/Analysi
[clang] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access (PR #108669)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/108669 Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe. >From 241fde13679b85e63126318520cd3f19029f49ad Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 13 Sep 2024 19:01:57 -0700 Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe. --- .../Checkers/WebKit/ASTUtils.cpp | 15 - .../Checkers/WebKit/PtrTypesSemantics.cpp | 5 ++--- .../Checkers/WebKit/PtrTypesSemantics.h | 4 ++-- .../Checkers/WebKit/uncounted-obj-arg.mm | 22 +++ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index be07cf51eefb3d..394cb26f03cf99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -12,6 +12,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/ExprObjC.h" #include namespace clang { @@ -35,6 +36,12 @@ bool tryToFindPtrOrigin( break; } } +if (auto *POE = dyn_cast(E)) { + if (auto *RF = POE->getResultExpr()) { +E = RF; +continue; + } +} if (auto *tempExpr = dyn_cast(E)) { E = tempExpr->getSubExpr(); continue; @@ -88,7 +95,7 @@ bool tryToFindPtrOrigin( continue; } -if (isReturnValueRefCounted(callee)) +if (isRefType(callee->getReturnType())) return callback(E, true); if (isSingleton(callee)) @@ -100,6 +107,12 @@ bool tryToFindPtrOrigin( } } } +if (auto *ObjCMsgExpr = dyn_cast(E)) { + if (auto *Method = ObjCMsgExpr->getMethodDecl()) { +if (isRefType(Method->getReturnType())) + return callback(E, true); + } +} if (auto *unaryOp = dyn_cast(E)) { // FIXME: Currently accepts ANY unary operator. Is it OK? E = unaryOp->getSubExpr(); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..9da3e54e454317 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -123,9 +123,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) { || FunctionName == "Identifier"; } -bool isReturnValueRefCounted(const clang::FunctionDecl *F) { - assert(F); - QualType type = F->getReturnType(); +bool isRefType(const clang::QualType T) { + QualType type = T; while (!type.isNull()) { if (auto *elaboratedT = type->getAs()) { type = elaboratedT->desugar(); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 2932e62ad06e4b..e2d0342bebd52c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -62,8 +62,8 @@ bool isRefType(const std::string &Name); /// false if not. bool isCtorOfRefCounted(const clang::FunctionDecl *F); -/// \returns true if \p F returns a ref-counted object, false if not. -bool isReturnValueRefCounted(const clang::FunctionDecl *F); +/// \returns true if \p T is RefPtr, Ref, or its variant, false if not. +bool isRefType(const clang::QualType T); /// \returns true if \p M is getter of a ref-counted class, false if not. std::optional isGetterOfRefCounted(const clang::CXXMethodDecl* Method); diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm index db0c5b19eec5bb..1d81a8851ece0c 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm @@ -11,6 +11,7 @@ @interface Foo : NSObject - (void)execute; - (RefPtr)_protectedRefCountable; +- (Ref)_protectedRefCountable2; @end @implementation Foo @@ -23,4 +24,25 @@ - (void)execute { return _countable; } +- (Ref)_protectedRefCountable2 { + return *_countable; +} + @end + +class RefCountedObject { +public: + void ref() const; + void deref() const; + Ref copy() const; +}; + +@interface WrapperObj : NSObject + +- (Ref)_protectedWebExtensionControllerConfiguration; + +@end + +static void foo(WrapperObj *configuration) { + configuration._protectedWebExtensionControllerConfiguration->copy(); +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings (PR #108656)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/108656 Improve the fix in 203a2ca8cd6af505e11a38aebceeaf864271042c by allowing variable references and more ignoring of parentheses. >From e86f101c5cd87db597c5fc8ab017b20adba98284 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Fri, 13 Sep 2024 15:19:45 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] ThreadSafeRefCounted still generates warnings Improve the fix in 203a2ca8cd6af505e11a38aebceeaf864271042c by allowing variable references and more ignoring of parentheses. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 19 ++- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 18 ++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index ecba5f9aa23ee3..5ea19233bb65c2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -84,13 +84,22 @@ class DerefFuncDeleteExprVisitor E = E->IgnoreParenCasts(); if (auto *TempE = dyn_cast(E)) E = TempE->getSubExpr(); +E = E->IgnoreParenCasts(); +if (auto *Ref = dyn_cast(E)) { + if (auto *Decl = Ref->getDecl()) { +if (auto *VD = dyn_cast(Decl)) + return VisitLabmdaArgument(VD->getInit()); + } + return false; +} +if (auto *Lambda = dyn_cast(E)) { + if (VisitBody(Lambda->getBody())) +return true; +} if (auto *ConstructE = dyn_cast(E)) { for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { -auto *Arg = ConstructE->getArg(i); -if (auto *Lambda = dyn_cast(Arg)) { - if (VisitBody(Lambda->getBody())) -return true; -} +if (VisitLabmdaArgument(ConstructE->getArg(i))) + return true; } } return false; diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp index 01527addb52992..33c60ea8ca64d1 100644 --- a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -119,6 +119,11 @@ template ensureOnMainThread([this] { delete static_cast(this); }); +} else if constexpr (destructionThread == DestructionThread::MainRunLoop) { +auto deleteThis = [this] { +delete static_cast(this); +}; +ensureOnMainThread(deleteThis); } } @@ -230,3 +235,16 @@ class FancyRefCountedClass4 final : public BadNestedThreadSafeRefCounted { +public: +static Ref create() +{ +return adoptRef(*new FancyRefCountedClass5()); +} + +virtual ~FancyRefCountedClass5(); + +private: +FancyRefCountedClass5(); +}; ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef (PR #108352)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108352 >From ac0447762c98da3cfb41a6b462034e3ab410bc33 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 12 Sep 2024 02:13:12 -0700 Subject: [PATCH 1/4] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../StaticAnalyzer/Checkers/CMakeLists.txt| 2 +- .../Checkers/WebKit/PtrTypesSemantics.cpp | 37 ++-- .../Checkers/WebKit/PtrTypesSemantics.h | 7 ++ ...Checker.cpp => RawPtrRefMemberChecker.cpp} | 88 --- .../Analysis/Checkers/WebKit/mock-types.h | 48 ++ .../Checkers/WebKit/unchecked-members.cpp | 53 +++ .../lib/StaticAnalyzer/Checkers/BUILD.gn | 2 +- 8 files changed, 219 insertions(+), 22 deletions(-) rename clang/lib/StaticAnalyzer/Checkers/WebKit/{NoUncountedMembersChecker.cpp => RawPtrRefMemberChecker.cpp} (66%) create mode 100644 clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 585246547b3dce..4759f680fb4ff7 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, + HelpText<"Check for no unchecked member variables.">, + Documentation; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 414282d58f779f..6da3665ab9a4df 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers VLASizeChecker.cpp ValistChecker.cpp VirtualCallChecker.cpp - WebKit/NoUncountedMembersChecker.cpp + WebKit/RawPtrRefMemberChecker.cpp WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..09298102993f99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -53,7 +53,9 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr; } -std::optional isRefCountable(const CXXRecordDecl* R) +std::optional isSmartPtrCompatible(const CXXRecordDecl *R, + const char *IncMethodName, + const char *DecMethodName) { assert(R); @@ -61,8 +63,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) if (!R) return std::nullopt; - bool hasRef = hasPublicMethodInBaseClass(R, "ref"); - bool hasDeref = hasPublicMethodInBaseClass(R, "deref"); + bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName); + bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName); if (hasRef && hasDeref) return true; @@ -71,8 +73,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) bool AnyInconclusiveBase = false; const auto hasPublicRefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName); if (!hasRefInBase) { AnyInconclusiveBase = true; return false; @@ -87,8 +89,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) Paths.clear(); const auto hasPublicDerefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName); if (!hasDerefInBase) { AnyInconclusiveBase = true; return false; @@ -103,11 +105,23 @@ std::optional isRefCountable(const CXXRecordDecl* R) return hasRef && hasDeref; } +std::optional isRefCountable(const clang::CXXRecordDecl *R) { + return isSmartPtrCompatible(R, "ref", "deref"); +} + +st
[clang] [llvm] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef (PR #108352)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108352 >From ac0447762c98da3cfb41a6b462034e3ab410bc33 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 12 Sep 2024 02:13:12 -0700 Subject: [PATCH 1/3] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../StaticAnalyzer/Checkers/CMakeLists.txt| 2 +- .../Checkers/WebKit/PtrTypesSemantics.cpp | 37 ++-- .../Checkers/WebKit/PtrTypesSemantics.h | 7 ++ ...Checker.cpp => RawPtrRefMemberChecker.cpp} | 88 --- .../Analysis/Checkers/WebKit/mock-types.h | 48 ++ .../Checkers/WebKit/unchecked-members.cpp | 53 +++ .../lib/StaticAnalyzer/Checkers/BUILD.gn | 2 +- 8 files changed, 219 insertions(+), 22 deletions(-) rename clang/lib/StaticAnalyzer/Checkers/WebKit/{NoUncountedMembersChecker.cpp => RawPtrRefMemberChecker.cpp} (66%) create mode 100644 clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 585246547b3dce..4759f680fb4ff7 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, + HelpText<"Check for no unchecked member variables.">, + Documentation; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 414282d58f779f..6da3665ab9a4df 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers VLASizeChecker.cpp ValistChecker.cpp VirtualCallChecker.cpp - WebKit/NoUncountedMembersChecker.cpp + WebKit/RawPtrRefMemberChecker.cpp WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..09298102993f99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -53,7 +53,9 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr; } -std::optional isRefCountable(const CXXRecordDecl* R) +std::optional isSmartPtrCompatible(const CXXRecordDecl *R, + const char *IncMethodName, + const char *DecMethodName) { assert(R); @@ -61,8 +63,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) if (!R) return std::nullopt; - bool hasRef = hasPublicMethodInBaseClass(R, "ref"); - bool hasDeref = hasPublicMethodInBaseClass(R, "deref"); + bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName); + bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName); if (hasRef && hasDeref) return true; @@ -71,8 +73,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) bool AnyInconclusiveBase = false; const auto hasPublicRefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName); if (!hasRefInBase) { AnyInconclusiveBase = true; return false; @@ -87,8 +89,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) Paths.clear(); const auto hasPublicDerefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName); if (!hasDerefInBase) { AnyInconclusiveBase = true; return false; @@ -103,11 +105,23 @@ std::optional isRefCountable(const CXXRecordDecl* R) return hasRef && hasDeref; } +std::optional isRefCountable(const clang::CXXRecordDecl *R) { + return isSmartPtrCompatible(R, "ref", "deref"); +} + +st
[clang] [llvm] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef (PR #108352)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108352 >From ac0447762c98da3cfb41a6b462034e3ab410bc33 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 12 Sep 2024 02:13:12 -0700 Subject: [PATCH 1/2] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../StaticAnalyzer/Checkers/CMakeLists.txt| 2 +- .../Checkers/WebKit/PtrTypesSemantics.cpp | 37 ++-- .../Checkers/WebKit/PtrTypesSemantics.h | 7 ++ ...Checker.cpp => RawPtrRefMemberChecker.cpp} | 88 --- .../Analysis/Checkers/WebKit/mock-types.h | 48 ++ .../Checkers/WebKit/unchecked-members.cpp | 53 +++ .../lib/StaticAnalyzer/Checkers/BUILD.gn | 2 +- 8 files changed, 219 insertions(+), 22 deletions(-) rename clang/lib/StaticAnalyzer/Checkers/WebKit/{NoUncountedMembersChecker.cpp => RawPtrRefMemberChecker.cpp} (66%) create mode 100644 clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 585246547b3dce..4759f680fb4ff7 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, + HelpText<"Check for no unchecked member variables.">, + Documentation; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 414282d58f779f..6da3665ab9a4df 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers VLASizeChecker.cpp ValistChecker.cpp VirtualCallChecker.cpp - WebKit/NoUncountedMembersChecker.cpp + WebKit/RawPtrRefMemberChecker.cpp WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..09298102993f99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -53,7 +53,9 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr; } -std::optional isRefCountable(const CXXRecordDecl* R) +std::optional isSmartPtrCompatible(const CXXRecordDecl *R, + const char *IncMethodName, + const char *DecMethodName) { assert(R); @@ -61,8 +63,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) if (!R) return std::nullopt; - bool hasRef = hasPublicMethodInBaseClass(R, "ref"); - bool hasDeref = hasPublicMethodInBaseClass(R, "deref"); + bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName); + bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName); if (hasRef && hasDeref) return true; @@ -71,8 +73,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) bool AnyInconclusiveBase = false; const auto hasPublicRefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName); if (!hasRefInBase) { AnyInconclusiveBase = true; return false; @@ -87,8 +89,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) Paths.clear(); const auto hasPublicDerefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName); if (!hasDerefInBase) { AnyInconclusiveBase = true; return false; @@ -103,11 +105,23 @@ std::optional isRefCountable(const CXXRecordDecl* R) return hasRef && hasDeref; } +std::optional isRefCountable(const clang::CXXRecordDecl *R) { + return isSmartPtrCompatible(R, "ref", "deref"); +} + +st
[clang] [llvm] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef (PR #108352)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/108352 This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. >From ac0447762c98da3cfb41a6b462034e3ab410bc33 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 12 Sep 2024 02:13:12 -0700 Subject: [PATCH] [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../StaticAnalyzer/Checkers/CMakeLists.txt| 2 +- .../Checkers/WebKit/PtrTypesSemantics.cpp | 37 ++-- .../Checkers/WebKit/PtrTypesSemantics.h | 7 ++ ...Checker.cpp => RawPtrRefMemberChecker.cpp} | 88 --- .../Analysis/Checkers/WebKit/mock-types.h | 48 ++ .../Checkers/WebKit/unchecked-members.cpp | 53 +++ .../lib/StaticAnalyzer/Checkers/BUILD.gn | 2 +- 8 files changed, 219 insertions(+), 22 deletions(-) rename clang/lib/StaticAnalyzer/Checkers/WebKit/{NoUncountedMembersChecker.cpp => RawPtrRefMemberChecker.cpp} (66%) create mode 100644 clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 585246547b3dce..4759f680fb4ff7 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, + HelpText<"Check for no unchecked member variables.">, + Documentation; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 414282d58f779f..6da3665ab9a4df 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers VLASizeChecker.cpp ValistChecker.cpp VirtualCallChecker.cpp - WebKit/NoUncountedMembersChecker.cpp + WebKit/RawPtrRefMemberChecker.cpp WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index f48b2fd9dca71b..09298102993f99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -53,7 +53,9 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr; } -std::optional isRefCountable(const CXXRecordDecl* R) +std::optional isSmartPtrCompatible(const CXXRecordDecl *R, + const char *IncMethodName, + const char *DecMethodName) { assert(R); @@ -61,8 +63,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) if (!R) return std::nullopt; - bool hasRef = hasPublicMethodInBaseClass(R, "ref"); - bool hasDeref = hasPublicMethodInBaseClass(R, "deref"); + bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName); + bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName); if (hasRef && hasDeref) return true; @@ -71,8 +73,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) bool AnyInconclusiveBase = false; const auto hasPublicRefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName); if (!hasRefInBase) { AnyInconclusiveBase = true; return false; @@ -87,8 +89,8 @@ std::optional isRefCountable(const CXXRecordDecl* R) Paths.clear(); const auto hasPublicDerefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { -auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref"); + [&](const CXXBaseSpecifier *Base, CXXBasePath &) { +auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName); if (!hasDerefInBase) { AnyInconclusiveBase = true; return false; @@ -103,11 +105,23 @@ std::optional isRefCountable(const CXX
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/108184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
rniwa wrote: Thanks for the review! https://github.com/llvm/llvm-project/pull/108184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
@@ -143,6 +143,16 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { return false; } +std::optional isUncounted(const QualType T) { + if (auto *Subst = dyn_cast(T)) { +if (auto *Decl = Subst->getAssociatedDecl()) { + if (isRefType(safeGetName(Decl))) +return false; +} + } + return isUncounted(T->getAsCXXRecordDecl()); +} + std::optional isUncounted(const CXXRecordDecl* Class) rniwa wrote: So the only other place where this function is called is in `isUncountedPtr` but that function needs to evaluate whether the pointee is ref counted or not so I don't think we can use the new function. https://github.com/llvm/llvm-project/pull/108184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebKit Checkers] Allow "singleton" suffix to be camelCased. (PR #108257)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/108257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
@@ -143,6 +143,16 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { return false; } +std::optional isUncounted(const clang::QualType T) { rniwa wrote: Fixed! https://github.com/llvm/llvm-project/pull/108184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108184 >From c8cd18baa5b285262905ad0d8c49ba102993ef1e Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 03:14:31 -0700 Subject: [PATCH 1/3] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 11 .../Checkers/WebKit/PtrTypesSemantics.h | 5 .../WebKit/UncountedCallArgsChecker.cpp | 3 +-- .../Checkers/WebKit/uncounted-obj-arg.mm | 26 +++ 4 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 49bbff1942167b..5a484d0546e95f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,6 +143,17 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { return false; } +std::optional isUncounted(const clang::QualType T) +{ + if (auto *Subst = dyn_cast(T)) { +if (auto *Decl = Subst->getAssociatedDecl()) { + if (isRefType(safeGetName(Decl))) +return false; +} + } + return isUncounted(T->getAsCXXRecordDecl()); +} + std::optional isUncounted(const CXXRecordDecl* Class) { // Keep isRefCounted first as it's cheaper. diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index ec1db1cc335807..2932e62ad06e4b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -20,6 +20,7 @@ class CXXMethodDecl; class CXXRecordDecl; class Decl; class FunctionDecl; +class QualType; class Stmt; class Type; @@ -42,6 +43,10 @@ std::optional isRefCountable(const clang::CXXRecordDecl* Class); /// \returns true if \p Class is ref-counted, false if not. bool isRefCounted(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is ref-countable AND not ref-counted, false if +/// not, std::nullopt if inconclusive. +std::optional isUncounted(const clang::QualType T); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional isUncounted(const clang::CXXRecordDecl* Class); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 704c082a4d1d63..81c2434ce64775 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -87,8 +87,7 @@ class UncountedCallArgsChecker } auto *E = MemberCallExpr->getImplicitObjectArgument(); QualType ArgType = MemberCallExpr->getObjectType(); -std::optional IsUncounted = -isUncounted(ArgType->getAsCXXRecordDecl()); +std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) reportBugOnThis(E); } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm new file mode 100644 index 00..db0c5b19eec5bb --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm @@ -0,0 +1,26 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s +// expected-no-diagnostics + +#import "mock-types.h" +#import "mock-system-header.h" +#import "../../Inputs/system-header-simulator-for-objc-dealloc.h" + +@interface Foo : NSObject + +@property (nonatomic, readonly) RefPtr countable; + +- (void)execute; +- (RefPtr)_protectedRefCountable; +@end + +@implementation Foo + +- (void)execute { + self._protectedRefCountable->method(); +} + +- (RefPtr)_protectedRefCountable { + return _countable; +} + +@end >From a598a8f2661bdd6163e2dddbeccb3bbada67ea16 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 09:32:19 -0700 Subject: [PATCH 2/3] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 5a484d0546e95f..25aaa5c5fdd900 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,8 +143,7 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { re
[clang] [WebKit Static Analyzer] Treat WTFReportBacktrace as a trivial function. (PR #108167)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/108167 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/108184 >From c8cd18baa5b285262905ad0d8c49ba102993ef1e Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 03:14:31 -0700 Subject: [PATCH 1/2] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 11 .../Checkers/WebKit/PtrTypesSemantics.h | 5 .../WebKit/UncountedCallArgsChecker.cpp | 3 +-- .../Checkers/WebKit/uncounted-obj-arg.mm | 26 +++ 4 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 49bbff1942167b..5a484d0546e95f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,6 +143,17 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { return false; } +std::optional isUncounted(const clang::QualType T) +{ + if (auto *Subst = dyn_cast(T)) { +if (auto *Decl = Subst->getAssociatedDecl()) { + if (isRefType(safeGetName(Decl))) +return false; +} + } + return isUncounted(T->getAsCXXRecordDecl()); +} + std::optional isUncounted(const CXXRecordDecl* Class) { // Keep isRefCounted first as it's cheaper. diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index ec1db1cc335807..2932e62ad06e4b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -20,6 +20,7 @@ class CXXMethodDecl; class CXXRecordDecl; class Decl; class FunctionDecl; +class QualType; class Stmt; class Type; @@ -42,6 +43,10 @@ std::optional isRefCountable(const clang::CXXRecordDecl* Class); /// \returns true if \p Class is ref-counted, false if not. bool isRefCounted(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is ref-countable AND not ref-counted, false if +/// not, std::nullopt if inconclusive. +std::optional isUncounted(const clang::QualType T); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional isUncounted(const clang::CXXRecordDecl* Class); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 704c082a4d1d63..81c2434ce64775 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -87,8 +87,7 @@ class UncountedCallArgsChecker } auto *E = MemberCallExpr->getImplicitObjectArgument(); QualType ArgType = MemberCallExpr->getObjectType(); -std::optional IsUncounted = -isUncounted(ArgType->getAsCXXRecordDecl()); +std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) reportBugOnThis(E); } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm new file mode 100644 index 00..db0c5b19eec5bb --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm @@ -0,0 +1,26 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s +// expected-no-diagnostics + +#import "mock-types.h" +#import "mock-system-header.h" +#import "../../Inputs/system-header-simulator-for-objc-dealloc.h" + +@interface Foo : NSObject + +@property (nonatomic, readonly) RefPtr countable; + +- (void)execute; +- (RefPtr)_protectedRefCountable; +@end + +@implementation Foo + +- (void)execute { + self._protectedRefCountable->method(); +} + +- (RefPtr)_protectedRefCountable { + return _countable; +} + +@end >From a598a8f2661bdd6163e2dddbeccb3bbada67ea16 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 09:32:19 -0700 Subject: [PATCH 2/2] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 5a484d0546e95f..25aaa5c5fdd900 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,8 +143,7 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { re
[clang] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ (PR #108184)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/108184 This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe. >From c8cd18baa5b285262905ad0d8c49ba102993ef1e Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 03:14:31 -0700 Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++ This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 11 .../Checkers/WebKit/PtrTypesSemantics.h | 5 .../WebKit/UncountedCallArgsChecker.cpp | 3 +-- .../Checkers/WebKit/uncounted-obj-arg.mm | 26 +++ 4 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 49bbff1942167b..5a484d0546e95f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,6 +143,17 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) { return false; } +std::optional isUncounted(const clang::QualType T) +{ + if (auto *Subst = dyn_cast(T)) { +if (auto *Decl = Subst->getAssociatedDecl()) { + if (isRefType(safeGetName(Decl))) +return false; +} + } + return isUncounted(T->getAsCXXRecordDecl()); +} + std::optional isUncounted(const CXXRecordDecl* Class) { // Keep isRefCounted first as it's cheaper. diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index ec1db1cc335807..2932e62ad06e4b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -20,6 +20,7 @@ class CXXMethodDecl; class CXXRecordDecl; class Decl; class FunctionDecl; +class QualType; class Stmt; class Type; @@ -42,6 +43,10 @@ std::optional isRefCountable(const clang::CXXRecordDecl* Class); /// \returns true if \p Class is ref-counted, false if not. bool isRefCounted(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is ref-countable AND not ref-counted, false if +/// not, std::nullopt if inconclusive. +std::optional isUncounted(const clang::QualType T); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional isUncounted(const clang::CXXRecordDecl* Class); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp index 704c082a4d1d63..81c2434ce64775 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -87,8 +87,7 @@ class UncountedCallArgsChecker } auto *E = MemberCallExpr->getImplicitObjectArgument(); QualType ArgType = MemberCallExpr->getObjectType(); -std::optional IsUncounted = -isUncounted(ArgType->getAsCXXRecordDecl()); +std::optional IsUncounted = isUncounted(ArgType); if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E)) reportBugOnThis(E); } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm new file mode 100644 index 00..db0c5b19eec5bb --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm @@ -0,0 +1,26 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s +// expected-no-diagnostics + +#import "mock-types.h" +#import "mock-system-header.h" +#import "../../Inputs/system-header-simulator-for-objc-dealloc.h" + +@interface Foo : NSObject + +@property (nonatomic, readonly) RefPtr countable; + +- (void)execute; +- (RefPtr)_protectedRefCountable; +@end + +@implementation Foo + +- (void)execute { + self._protectedRefCountable->method(); +} + +- (RefPtr)_protectedRefCountable { + return _countable; +} + +@end ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebKit Static Analyzer] Treat WTFReportBacktrace as a trivial function. (PR #108167)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/108167 Treat WTFReportBacktrace, which prints out the backtrace, as trivial. >From ad40cdfa22ccbbea7f8b67bf0f3ac1cc0ce1a46c Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Wed, 11 Sep 2024 01:24:27 -0700 Subject: [PATCH] [WebKit Static Analyzer] Treat WTFReportBacktrace as a trivial function. Treat WTFReportBacktrace, which prints out the backtrace, as trivial. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 1 + clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp | 3 +++ 2 files changed, 4 insertions(+) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 49bbff1942167b..2b9b7883c978ba 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -397,6 +397,7 @@ class TrivialFunctionAnalysisVisitor return true; if (Name == "WTFCrashWithInfo" || Name == "WTFBreakpointTrap" || +Name == "WTFReportBacktrace" || Name == "WTFCrashWithSecurityImplication" || Name == "WTFCrash" || Name == "WTFReportAssertionFailure" || Name == "isMainThread" || Name == "isMainThreadOrGCThread" || Name == "isMainRunLoop" || diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index a98c6eb9c84d97..424ebd349e955a 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -6,6 +6,7 @@ void WTFBreakpointTrap(); void WTFCrashWithInfo(int, const char*, const char*, int); void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion); +void WTFReportBacktrace(void); void WTFCrash(void); void WTFCrashWithSecurityImplication(void); @@ -334,6 +335,7 @@ class RefCounted { } unsigned trivial60() { return ObjectWithNonTrivialDestructor { 5 }.value(); } unsigned trivial61() { return DerivedNumber('7').value(); } + void trivial62() { WTFReportBacktrace(); } static RefCounted& singleton() { static RefCounted s_RefCounted; @@ -506,6 +508,7 @@ class UnrelatedClass { getFieldTrivial().trivial59(); // no-warning getFieldTrivial().trivial60(); // no-warning getFieldTrivial().trivial61(); // no-warning +getFieldTrivial().trivial62(); // no-warning RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676 >From 516ba7d30880f9aa2a0bf6ed884f5d5541b430ce Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 10 Sep 2024 19:03:12 -0700 Subject: [PATCH] This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to ensureOnMainThread or ensureOnMainRunLoop. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 26 ++ .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 258 insertions(+) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 9df108e28ecdbb..ecba5f9aa23ee3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -67,6 +67,32 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (VisitLabmdaArgument(Arg)) +return true; +} + } +} +return false; + } + + bool VisitLabmdaArgument(const Expr *E) { +E = E->IgnoreParenCasts(); +if (auto *TempE = dyn_cast(E)) + E = TempE->getSubExpr(); +if (auto *ConstructE = dyn_cast(E)) { + for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { +auto *Arg = ConstructE->getArg(i); +if (auto *Lambda = dyn_cast(Arg)) { + if (VisitBody(Lambda->getBody())) +return true; +} + } +} return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..01527addb52992 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrapperBase { +public: +explicit CallableWrapper(CallableType&& callable) +: m_callable(WTFMove(callable)) { } +CallableWrapper(const CallableWrapper&) = delete; +CallableWrapper& operator=(const CallableWrapper&) = delete; +Out call(In... in) final; +private: +CallableType m_callable; +}; + +} // namespace Detail + +template class Function; + +template Function adopt(Detail::CallableWrapperBase*); + +template +class Function { +public: +using Impl = Detail::CallableWrapperBase; + +Function() = default; + +template +Function(FunctionType f); + +Out operator()(In... in) const; +explicit operator bool() const { return !!m_callableWrapper; } + +private: +enum AdoptTag { Adopt }; +Function(Impl* impl, AdoptTag) +: m_callableWrapper(impl) +{ +} + +friend Function adopt(Impl*); + +Impl* m_callableWrapper; +}; + +template Function adopt(Detail::CallableWrapperBase* impl) +{ +return Function(impl, Function::Adopt); +} + +template, typename RefDerefTraits = DefaultRefDerefTraits> Ref adoptRef(T&); + +template +inline Ref adoptRef(T& reference) +{ +return Ref(reference); +} + +enum class DestructionThread : unsigned char { Any, Main, MainRunLoop }; +void ensureOnMainThread(Function&&); // Sync if called on main thread, async otherwise. +void ensureOnMainRunLoop(Function&&); // Sync if called on main run loop, async otherwise. + +class ThreadSafeRefCountedBase { +public: +ThreadSafeRefCountedBase() = default; + +void ref() const +{ +++m_refCount; +} + +bool hasOneRef(
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676 >From 3a5031d022f01baaf6fd96b2c2c0891e9b627d2c Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 10 Sep 2024 19:03:12 -0700 Subject: [PATCH] This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to ensureOnMainThread or ensureOnMainRunLoop. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 27 ++ .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 259 insertions(+) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 9df108e28ecdbb..fd0a60c30bb24a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -17,6 +17,7 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SetVector.h" #include @@ -67,6 +68,32 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (VisitLabmdaArgument(Arg)) +return true; +} + } +} +return false; + } + + bool VisitLabmdaArgument(const Expr *E) { +E = E->IgnoreParenCasts(); +if (auto *TempE = dyn_cast(E)) + E = TempE->getSubExpr(); +if (auto *ConstructE = dyn_cast(E)) { + for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { +auto *Arg = ConstructE->getArg(i); +if (auto *Lambda = dyn_cast(Arg)) { + if (VisitBody(Lambda->getBody())) +return true; +} + } +} return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..01527addb52992 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrapperBase { +public: +explicit CallableWrapper(CallableType&& callable) +: m_callable(WTFMove(callable)) { } +CallableWrapper(const CallableWrapper&) = delete; +CallableWrapper& operator=(const CallableWrapper&) = delete; +Out call(In... in) final; +private: +CallableType m_callable; +}; + +} // namespace Detail + +template class Function; + +template Function adopt(Detail::CallableWrapperBase*); + +template +class Function { +public: +using Impl = Detail::CallableWrapperBase; + +Function() = default; + +template +Function(FunctionType f); + +Out operator()(In... in) const; +explicit operator bool() const { return !!m_callableWrapper; } + +private: +enum AdoptTag { Adopt }; +Function(Impl* impl, AdoptTag) +: m_callableWrapper(impl) +{ +} + +friend Function adopt(Impl*); + +Impl* m_callableWrapper; +}; + +template Function adopt(Detail::CallableWrapperBase* impl) +{ +return Function(impl, Function::Adopt); +} + +template, typename RefDerefTraits = DefaultRefDerefTraits> Ref adoptRef(T&); + +template +inline Ref adoptRef(T& reference) +{ +return Ref(reference); +} + +enum class DestructionThread : unsigned char { Any, Main, MainRunLoop }; +void ensureOnMainThread(Function&&); // Sync if called on main thread, async otherwise. +void ensureOnMainRunLoop(F
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
@@ -67,6 +68,48 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (FindLabmda(Arg)) +return true; +} + } +} +return false; + } + + bool FindLabmda(const Expr *E) { +while (E) { + if (auto *TempE = dyn_cast(E)) { rniwa wrote: Simplified this code quite a bit using `IgnoreParenCasts`. https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676 >From cb214cc1f922e16ea4bd81f3c9cac54bc97c2968 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 10 Sep 2024 19:03:12 -0700 Subject: [PATCH] This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to ensureOnMainThread or ensureOnMainRunLoop. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 31 ++- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 9df108e28ecdbb..8228a1962e5bab 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -17,6 +17,7 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SetVector.h" #include @@ -67,6 +68,32 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (VisitLabmdaArgument(Arg)) +return true; +} + } +} +return false; + } + + bool VisitLabmdaArgument(const Expr *E) { +E = E->IgnoreParenCasts(); +if (auto *TempE = dyn_cast(E)) + E = TempE->getSubExpr(); +if (auto *ConstructE = dyn_cast(E)) { + for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { +auto *Arg = ConstructE->getArg(i); +if (auto *Lambda = dyn_cast(Arg)) { + if (VisitBody(Lambda->getBody())) +return true; +} + } +} return false; } @@ -113,7 +140,9 @@ class DerefFuncDeleteExprVisitor // Return false since the contents of lambda isn't necessarily executed. // If it is executed, VisitCallExpr above will visit its body. - bool VisitLambdaExpr(const LambdaExpr *) { return false; } + // Allow returning true for a lambda if it's a function argument to another + // function without body / definition. + bool VisitLambdaExpr(const LambdaExpr *E) { return false; } private: const TemplateArgumentList *ArgList{nullptr}; diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..01527addb52992 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrapperBase { +public: +explicit CallableWrapper(CallableType&& callable) +: m_callable(WTFMove(callable)) { } +CallableWrapper(const CallableWrapper&) = delete; +CallableWrapper& operator=(const CallableWrapper&) = delete; +Out call(In... in) final; +private: +CallableType m_callable; +}; + +} // namespace Detail + +template class Function; + +template Function adopt(Detail::CallableWrapperBase*); + +template +class Function { +public: +using Impl = Detail::CallableWrapperBase; + +Function() = default; + +template +Function(FunctionType f); + +Out operator()(In... in) const; +explicit operator bool() const { return !!m_callableWrapper; } + +private: +enum AdoptTag { Adopt }; +Function(Impl* impl, AdoptTag) +: m_callableWrapper(impl) +{ +} + +friend Function
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
@@ -67,6 +68,48 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (FindLabmda(Arg)) +return true; +} + } +} +return false; + } + + bool FindLabmda(const Expr *E) { +while (E) { + if (auto *TempE = dyn_cast(E)) { rniwa wrote: Ah, that's so nice! https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
rniwa wrote: > > Looks like you're putting no restrictions on what the opaque function is. > > This may cause some false negatives but it's probably ultimately ok, but it > > might be a good idea to confirm. > > Yes, we're not putting any requirement for the functions for now. Actually, why not. We can just require the function names to be ensureOnMainThread or ensureOnMainRunLoop for now. We can relax the requirement further if we find more cases. https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
@@ -67,6 +68,15 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else if (!VisitLambdaBody) { + for (unsigned i = 0; i < CE->getNumArgs(); ++i) { +auto *Arg = CE->getArg(i); +VisitLambdaBody = true; +auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = false; }); +if (VisitChildren(Arg)) rniwa wrote: Okay, we can do that. Updated PR accordingly. https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676 >From be0c2e0af8e06f4b5f33855a7020b5148cb1846c Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 10 Sep 2024 19:03:12 -0700 Subject: [PATCH] This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to ensureOnMainThread or ensureOnMainRunLoop. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 47 +++- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 9df108e28ecdbb..92ce391ea47bfe 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -17,6 +17,7 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SetVector.h" #include @@ -67,6 +68,48 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else { + auto name = safeGetName(D); + if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") { +for (unsigned i = 0; i < CE->getNumArgs(); ++i) { + auto *Arg = CE->getArg(i); + if (FindLabmda(Arg)) +return true; +} + } +} +return false; + } + + bool FindLabmda(const Expr *E) { +while (E) { + if (auto *TempE = dyn_cast(E)) { +E = TempE->getSubExpr(); +continue; + } + if (auto *TempE = dyn_cast(E)) { +E = TempE->getSubExpr(); +continue; + } + if (auto *ParenE = dyn_cast(E)) { +E = ParenE->getSubExpr(); +continue; + } + if (auto *CastE = dyn_cast(E)) { +E = CastE->getSubExpr(); +continue; + } + if (auto *ConstructE = dyn_cast(E)) { +for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) { + auto *Arg = ConstructE->getArg(i); + if (auto *Lambda = dyn_cast(Arg)) { +if (VisitBody(Lambda->getBody())) + return true; + } +} + } + break; +} return false; } @@ -113,7 +156,9 @@ class DerefFuncDeleteExprVisitor // Return false since the contents of lambda isn't necessarily executed. // If it is executed, VisitCallExpr above will visit its body. - bool VisitLambdaExpr(const LambdaExpr *) { return false; } + // Allow returning true for a lambda if it's a function argument to another + // function without body / definition. + bool VisitLambdaExpr(const LambdaExpr *E) { return false; } private: const TemplateArgumentList *ArgList{nullptr}; diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..01527addb52992 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrapperBase { +public: +explicit CallableWrapper(CallableType&& callable) +: m_callable(WTFMove(callable)) { } +CallableWrapper(const CallableWrapper&) = delete; +CallableWrapper& operator=(const CallableWrapper&) = delete; +Out call(In... in) final; +private: +CallableType m_callable; +}; + +} // namespace Detail + +template class Function; + +template Function adopt(Detail::CallableWrapperBase*); + +template +class Function { +public: +using Impl = Detail::Calla
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676 >From 9a8c60355f88d7d4cee6d9f75312bb87d13b74a9 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 7 Sep 2024 01:45:05 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to an "opaque" function; i.e. a function without definition / body. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 22 +- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 26879f2f87c8be..8e97efc1ab8e8e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -17,6 +17,7 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SetVector.h" #include @@ -67,6 +68,15 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else if (!VisitLambdaBody) { + for (unsigned i = 0; i < CE->getNumArgs(); ++i) { +auto *Arg = CE->getArg(i); +VisitLambdaBody = true; +auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = false; }); +if (VisitChildren(Arg)) + return true; + } +} return false; } @@ -113,12 +123,22 @@ class DerefFuncDeleteExprVisitor // Return false since the contents of lambda isn't necessarily executed. // If it is executed, VisitCallExpr above will visit its body. - bool VisitLambdaExpr(const LambdaExpr *) { return false; } + // Allow returning true for a lambda if it's a function argument to another + // function without body / definition. + bool VisitLambdaExpr(const LambdaExpr *E) { +if (VisitLambdaBody) { + VisitLambdaBody = false; + auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = true; }); + return VisitChildren(E); +} +return false; + } private: const TemplateArgumentList *ArgList{nullptr}; const CXXRecordDecl *ClassDecl; llvm::DenseSet VisitedBody; + bool VisitLambdaBody{false}; }; class RefCntblBaseVirtualDtorChecker diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..e149c1d3d3575f --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrapperBase { +public: +explicit CallableWrapper(CallableType&& callable) +: m_callable(WTFMove(callable)) { } +CallableWrapper(const CallableWrapper&) = delete; +CallableWrapper& operator=(const CallableWrapper&) = delete; +Out call(In... in) final; +private: +CallableType m_callable; +}; + +} // namespace Detail + +template class Function; + +template Function adopt(Detail::CallableWrapperBase*); + +template +class Function { +public: +using Impl = Detail::CallableWrapperBase; + +Function() = default; + +template +Function(FunctionType f); + +Out operator()(In... in) const; +explicit operator bool() const { return !!m_callableWrapper; } + +private: +enum AdoptTag { Adopt }; +Function(Impl* impl, AdoptTag) +: m_callableWrapper(impl) +{ +} + +friend Function adopt(Impl*); + +Impl* m_callab
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
rniwa wrote: > Looks like you're putting no restrictions on what the opaque function is. > This may cause some false negatives but it's probably ultimately ok, but it > might be a good idea to confirm. Yes, we're not putting any requirement for the functions for now. https://github.com/llvm/llvm-project/pull/107676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/107676 This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to an "opaque" function; i.e. a function without definition / body. >From 9a8c60355f88d7d4cee6d9f75312bb87d13b74a9 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 7 Sep 2024 01:45:05 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to an "opaque" function; i.e. a function without definition / body. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 22 +- .../ref-cntbl-crtp-base-no-virtual-dtor.cpp | 232 ++ 2 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 26879f2f87c8be..8e97efc1ab8e8e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -17,6 +17,7 @@ #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SetVector.h" #include @@ -67,6 +68,15 @@ class DerefFuncDeleteExprVisitor const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); +else if (!VisitLambdaBody) { + for (unsigned i = 0; i < CE->getNumArgs(); ++i) { +auto *Arg = CE->getArg(i); +VisitLambdaBody = true; +auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = false; }); +if (VisitChildren(Arg)) + return true; + } +} return false; } @@ -113,12 +123,22 @@ class DerefFuncDeleteExprVisitor // Return false since the contents of lambda isn't necessarily executed. // If it is executed, VisitCallExpr above will visit its body. - bool VisitLambdaExpr(const LambdaExpr *) { return false; } + // Allow returning true for a lambda if it's a function argument to another + // function without body / definition. + bool VisitLambdaExpr(const LambdaExpr *E) { +if (VisitLambdaBody) { + VisitLambdaBody = false; + auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = true; }); + return VisitChildren(E); +} +return false; + } private: const TemplateArgumentList *ArgList{nullptr}; const CXXRecordDecl *ClassDecl; llvm::DenseSet VisitedBody; + bool VisitLambdaBody{false}; }; class RefCntblBaseVirtualDtorChecker diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp new file mode 100644 index 00..e149c1d3d3575f --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp @@ -0,0 +1,232 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s + +#include "mock-types.h" + +namespace Detail { + +template +class CallableWrapperBase { +public: +virtual ~CallableWrapperBase() { } +virtual Out call(In...) = 0; +}; + +template class CallableWrapper; + +template +class CallableWrapper : public CallableWrap
[clang] Fix the warning in RefCntblBaseVirtualDtorChecker.cpp:61 (PR #93403)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/93403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,116 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefFuncDeleteExprVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefFuncDeleteExprVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefFuncDeleteExprVisitor(const CXXRecordDecl *ClassDecl) + : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) rniwa wrote: Here's a PR to fix it: https://github.com/llvm/llvm-project/pull/93403 https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the warning in RefCntblBaseVirtualDtorChecker.cpp:61 (PR #93403)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/93403 None >From 4f5b6ac39e709bddf7f1ced314ebb1984a1942de Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 26 May 2024 00:34:15 -0700 Subject: [PATCH] Fix the warning in RefCntblBaseVirtualDtorChecker.cpp:61 --- .../Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 26879f2f87c8b..9df108e28ecdb 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -58,7 +58,7 @@ class DerefFuncDeleteExprVisitor std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { if (auto *Body = Decl->getBody()) return VisitBody(Body); -if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) +if (Decl->getTemplateInstantiationPattern()) return std::nullopt; // Indeterminate. There was no concrete instance. return false; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,116 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefFuncDeleteExprVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefFuncDeleteExprVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefFuncDeleteExprVisitor(const CXXRecordDecl *ClassDecl) + : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) rniwa wrote: Oh oops, sorry about that. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92837 >From 9c2ae2b2b14d27270589f3775df95a7547e74c83 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH 1/4] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 + ...virtual-dtor-ref-deref-on-diff-classes.cpp | 1 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 3 files changed, 567 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..efb7b4456f2ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dy
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive rniwa wrote: Yeah. We could keep the visitor around for the duration of the checker & cache the results. Will address that in a follow up. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/91876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
rniwa wrote: Thanks for the review! https://github.com/llvm/llvm-project/pull/91876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/92639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92837 >From 9c2ae2b2b14d27270589f3775df95a7547e74c83 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH 1/3] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 + ...virtual-dtor-ref-deref-on-diff-classes.cpp | 1 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 3 files changed, 567 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..efb7b4456f2ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dy
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92837 >From 9c2ae2b2b14d27270589f3775df95a7547e74c83 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH 1/2] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 + ...virtual-dtor-ref-deref-on-diff-classes.cpp | 1 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 3 files changed, 567 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..efb7b4456f2ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dy
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive rniwa wrote: Actually, the only way we'd hit this code path is if the function is recursive and we're still in the middle of recursively visiting the function definition so the best we can do here is to return false (since we haven't finished traversing the function body yet). https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { rniwa wrote: Oh I see. Indeed, it looks like we can get away with just: ```cpp bool VisitCallExpr(const CallExpr *CE) { const Decl *D = CE->getCalleeDecl(); if (D && D->hasBody()) return VisitBody(D->getBody()); return false; } ``` https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From e40017a2750ee39bfd1a87b5ddea620076bc4419 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/6] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 5c797d5233089..2a4da9eeaee6d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -517,11 +517,9 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; @@ -535,12 +533,14 @@ bool TrivialFunctionAnalysis::isTrivialImpl( } const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 96986631726fe..18af9e17f78b0 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -231,6 +231,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -498,6 +500,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From 5d7a259c0209a8cbb70c2718518905669db2a885 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/6] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 2a4da9eeaee6d..449e56be9984d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -535,7 +535,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } bool Result = V.Visit(Body); >From 9971dcdebc160acb708b548bf67ef5efa76d4fe0 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/6] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 69 +++ .../Checkers/WebKit/uncounted-obj-arg.
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
@@ -231,6 +231,15 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveTrivialFunction(int n) { return !n ? 1 : recursiveTrivialFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } + unsigned mutuallyRecursiveFunction1(int n) { return n < 0 ? 1 : (n % 2 ? mutuallyRecursiveFunction2(n - 2) : mutuallyRecursiveFunction1(n - 1)); } + unsigned mutuallyRecursiveFunction2(int n) { return n < 0 ? 1 : (n % 3 ? mutuallyRecursiveFunction2(n - 3) : mutuallyRecursiveFunction1(n - 2)); } + unsigned mutuallyRecursiveFunction3(int n) { return n < 0 ? 1 : (n % 5 ? mutuallyRecursiveFunction3(n - 5) : mutuallyRecursiveFunction4(n - 3)); } + unsigned mutuallyRecursiveFunction4(int n) { return n < 0 ? 1 : (n % 7 ? otherFunction() : mutuallyRecursiveFunction3(n - 3)); } + unsigned mutuallyRecursiveFunction5(unsigned n) { return n > 100 ? 2 : (n % 2 ? mutuallyRecursiveFunction5(n + 1) : mutuallyRecursiveFunction6(n + 2)); } rniwa wrote: Sure, added. https://github.com/llvm/llvm-project/pull/91876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92639 >From 5ae3b193a6ec3617c99297da5b8d276b0e5bbc01 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 18 May 2024 02:17:30 -0700 Subject: [PATCH 1/2] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for assignments to uncounted local variable and parameters instead of just the initialization during the declaration. --- .../WebKit/UncountedLocalVarsChecker.cpp | 61 +++ .../Checkers/WebKit/uncounted-local-vars.cpp | 42 + 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 0d9710a5e2d83..6c0d56303d5ad 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -135,7 +135,19 @@ class UncountedLocalVarsChecker bool shouldVisitImplicitCode() const { return false; } bool VisitVarDecl(VarDecl *V) { -Checker->visitVarDecl(V); +auto *Init = V->getInit(); +if (Init && V->isLocalVarDecl()) + Checker->visitVarDecl(V, Init); +return true; + } + + bool VisitBinaryOperator(const BinaryOperator *BO) { +if (BO->isAssignmentOp()) { + if (auto *VarRef = dyn_cast(BO->getLHS())) { +if (auto *V = dyn_cast(VarRef->getDecl())) + Checker->visitVarDecl(V, BO->getRHS()); + } +} return true; } @@ -174,7 +186,7 @@ class UncountedLocalVarsChecker visitor.TraverseDecl(const_cast(TUD)); } - void visitVarDecl(const VarDecl *V) const { + void visitVarDecl(const VarDecl *V, const Expr *Value) const { if (shouldSkipVarDecl(V)) return; @@ -184,12 +196,8 @@ class UncountedLocalVarsChecker std::optional IsUncountedPtr = isUncountedPtr(ArgType); if (IsUncountedPtr && *IsUncountedPtr) { - const Expr *const InitExpr = V->getInit(); - if (!InitExpr) -return; // FIXME: later on we might warn on uninitialized vars too - if (tryToFindPtrOrigin( - InitExpr, /*StopAtFirstRefCountedObj=*/false, + Value, /*StopAtFirstRefCountedObj=*/false, [&](const clang::Expr *InitArgOrigin, bool IsSafe) { if (!InitArgOrigin) return true; @@ -232,34 +240,39 @@ class UncountedLocalVarsChecker })) return; - reportBug(V); + reportBug(V, Value); } } bool shouldSkipVarDecl(const VarDecl *V) const { assert(V); -if (!V->isLocalVarDecl()) - return true; - -if (BR->getSourceManager().isInSystemHeader(V->getLocation())) - return true; - -return false; +return BR->getSourceManager().isInSystemHeader(V->getLocation()); } - void reportBug(const VarDecl *V) const { + void reportBug(const VarDecl *V, const Expr *Value) const { assert(V); SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); -Os << "Local variable "; -printQuotedQualifiedName(Os, V); -Os << " is uncounted and unsafe."; - -PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager()); -auto Report = std::make_unique(Bug, Os.str(), BSLoc); -Report->addRange(V->getSourceRange()); -BR->emitReport(std::move(Report)); +if (dyn_cast(V)) { + Os << "Assignment to an uncounted parameter "; + printQuotedQualifiedName(Os, V); + Os << " is unsafe."; + + PathDiagnosticLocation BSLoc(Value->getExprLoc(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(Value->getSourceRange()); + BR->emitReport(std::move(Report)); +} else { + Os << "Local variable "; + printQuotedQualifiedName(Os, V); + Os << " is uncounted and unsafe."; + + PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(V->getSourceRange()); + BR->emitReport(std::move(Report)); +} } }; } // namespace diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 632a82eb0d8d1..abcb9c5122f70 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -216,3 +216,45 @@ void foo() { } } // namespace conditional_op + +namespace local_assignment_basic { + +RefCountable *provide_ref_ctnbl(); + +void foo(RefCountable* a) { + RefCountable* b = a; + // expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
rniwa wrote: > (It might be a good idea to add comments to those parts of the code to make > sure the reader knows that it was intentional.) Ok, I was gonna say I'd add tests and then realized that the error message says "local variable" to both static local as well as global variables. That's probably misleading so will correct that. https://github.com/llvm/llvm-project/pull/92639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor rniwa wrote: Hm... but it's important that the function in question is `deref()` though. In that, that's what guarantees the object to be eventually deleted appropriately. Maybe `DerefFuncDeleteExprVisitor`? https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { rniwa wrote: Sure, we can do that. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { rniwa wrote: What do you mean by `getInit()` branch is unnecessary? https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); rniwa wrote: Sure, we can do that. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive rniwa wrote: We could do that. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -51,92 +169,137 @@ class RefCntblBaseVirtualDtorChecker bool shouldVisitImplicitCode() const { return false; } bool VisitCXXRecordDecl(const CXXRecordDecl *RD) { -Checker->visitCXXRecordDecl(RD); +if (!RD->hasDefinition()) + return true; + +Decls.insert(RD); + +for (auto &Base : RD->bases()) { + const auto AccSpec = Base.getAccessSpecifier(); + if (AccSpec == AS_protected || AccSpec == AS_private || + (AccSpec == AS_none && RD->isClass())) +continue; + + QualType T = Base.getType(); + if (T.isNull()) +continue; + + const CXXRecordDecl *C = T->getAsCXXRecordDecl(); + if (!C) +continue; + + if (auto *CTSD = dyn_cast(C)) { +auto &Args = CTSD->getTemplateArgs(); +for (unsigned i = 0; i < Args.size(); ++i) { rniwa wrote: oh, neat. I'll go do that. https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
@@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor rniwa wrote: Oh, this is `deref` function (i.e. function by the name of `deref`) analysis visitor. Maybe I should call it `DerefFuncVisitor` instead? https://github.com/llvm/llvm-project/pull/92837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker bool shouldVisitImplicitCode() const { return false; } bool VisitVarDecl(VarDecl *V) { -Checker->visitVarDecl(V); +auto *Init = V->getInit(); +if (Init && V->isLocalVarDecl()) + Checker->visitVarDecl(V, Init); +return true; + } + + bool VisitBinaryOperator(const BinaryOperator *BO) { +if (BO->isAssignmentOp()) { + if (auto *VarRef = dyn_cast(BO->getLHS())) { +if (auto *V = dyn_cast(VarRef->getDecl())) rniwa wrote: Hm... I don't think so. I think any assignment to a global variable is also bad if it's a raw pointer to a ref counted type. It's generally an anti-pattern to avoid. This makes me think that the checker should probably be renamed to "UncountedVarDeclChecker" or something. https://github.com/llvm/llvm-project/pull/92639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker bool shouldVisitImplicitCode() const { return false; } bool VisitVarDecl(VarDecl *V) { -Checker->visitVarDecl(V); +auto *Init = V->getInit(); +if (Init && V->isLocalVarDecl()) rniwa wrote: Oh, interesting. I *think* we do want to check static local variables as well since there is nothing preventing static local variable to be a raw pointer to a ref counted type, which is also bad. https://github.com/llvm/llvm-project/pull/92639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92837 >From 9c2ae2b2b14d27270589f3775df95a7547e74c83 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 + ...virtual-dtor-ref-deref-on-diff-classes.cpp | 1 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 3 files changed, 567 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..efb7b4456f2ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_ca
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92837 >From 9cbdf8228c8b10f4c8cd4e8770b58921e559a687 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 2 files changed, 566 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..efb7b4456f2ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) + : ArgList(&ArgList), ClassDecl(ClassDecl) {} + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) {} + + std::optional HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Body = Decl->getBody()) + return VisitBody(Body); +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return std::nullopt; // Indeterminate. There was no concrete instance. +return false; + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
rniwa wrote: Closing this PR in favor of https://github.com/llvm/llvm-project/pull/92837, which has a single consolidated commit. https://github.com/llvm/llvm-project/pull/92501 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/92501 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. (PR #92837)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92837 Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. >From 6061b5e40b5a2ebf73c5ea29f781c910e147ef1f Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Mon, 20 May 2024 16:12:44 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy and transitively proves every ref-counted base class has a virtual destructor or deref function which casts this pointer back to the derived class before deleting. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 307 + .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +- 2 files changed, 562 insertions(+), 69 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..cd339911fbf38 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,134 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 01/10] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/9] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/8] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92639 This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for assignments to uncounted local variable and parameters instead of just the initialization during the declaration. >From 5ae3b193a6ec3617c99297da5b8d276b0e5bbc01 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 18 May 2024 02:17:30 -0700 Subject: [PATCH] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for assignments to uncounted local variable and parameters instead of just the initialization during the declaration. --- .../WebKit/UncountedLocalVarsChecker.cpp | 61 +++ .../Checkers/WebKit/uncounted-local-vars.cpp | 42 + 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 0d9710a5e2d83..6c0d56303d5ad 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -135,7 +135,19 @@ class UncountedLocalVarsChecker bool shouldVisitImplicitCode() const { return false; } bool VisitVarDecl(VarDecl *V) { -Checker->visitVarDecl(V); +auto *Init = V->getInit(); +if (Init && V->isLocalVarDecl()) + Checker->visitVarDecl(V, Init); +return true; + } + + bool VisitBinaryOperator(const BinaryOperator *BO) { +if (BO->isAssignmentOp()) { + if (auto *VarRef = dyn_cast(BO->getLHS())) { +if (auto *V = dyn_cast(VarRef->getDecl())) + Checker->visitVarDecl(V, BO->getRHS()); + } +} return true; } @@ -174,7 +186,7 @@ class UncountedLocalVarsChecker visitor.TraverseDecl(const_cast(TUD)); } - void visitVarDecl(const VarDecl *V) const { + void visitVarDecl(const VarDecl *V, const Expr *Value) const { if (shouldSkipVarDecl(V)) return; @@ -184,12 +196,8 @@ class UncountedLocalVarsChecker std::optional IsUncountedPtr = isUncountedPtr(ArgType); if (IsUncountedPtr && *IsUncountedPtr) { - const Expr *const InitExpr = V->getInit(); - if (!InitExpr) -return; // FIXME: later on we might warn on uninitialized vars too - if (tryToFindPtrOrigin( - InitExpr, /*StopAtFirstRefCountedObj=*/false, + Value, /*StopAtFirstRefCountedObj=*/false, [&](const clang::Expr *InitArgOrigin, bool IsSafe) { if (!InitArgOrigin) return true; @@ -232,34 +240,39 @@ class UncountedLocalVarsChecker })) return; - reportBug(V); + reportBug(V, Value); } } bool shouldSkipVarDecl(const VarDecl *V) const { assert(V); -if (!V->isLocalVarDecl()) - return true; - -if (BR->getSourceManager().isInSystemHeader(V->getLocation())) - return true; - -return false; +return BR->getSourceManager().isInSystemHeader(V->getLocation()); } - void reportBug(const VarDecl *V) const { + void reportBug(const VarDecl *V, const Expr *Value) const { assert(V); SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); -Os << "Local variable "; -printQuotedQualifiedName(Os, V); -Os << " is uncounted and unsafe."; - -PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager()); -auto Report = std::make_unique(Bug, Os.str(), BSLoc); -Report->addRange(V->getSourceRange()); -BR->emitReport(std::move(Report)); +if (dyn_cast(V)) { + Os << "Assignment to an uncounted parameter "; + printQuotedQualifiedName(Os, V); + Os << " is unsafe."; + + PathDiagnosticLocation BSLoc(Value->getExprLoc(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(Value->getSourceRange()); + BR->emitReport(std::move(Report)); +} else { + Os << "Local variable "; + printQuotedQualifiedName(Os, V); + Os << " is uncounted and unsafe."; + + PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(V->getSourceRange()); + BR->emitReport(std::move(Report)); +} } }; } // namespace diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 632a82eb0d8d1..abcb9c5122f70 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -216,3 +216,45 @@ void foo() { } } // namespace conditional_op + +namespace local_assignment_basic { + +RefCountable *p
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/7] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/6] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/5] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/4] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/3] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501 >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH 1/2] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { nullptr }; + const CXXRecordDecl* ClassDecl; + llvm::DenseSet VisitedBody; +}; + class RefCntblBaseVirtualDtorChecker : public Checker> { private: @@ -91,8 +2
[clang] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. (PR #92501)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92501 When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. >From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 16 May 2024 23:24:13 -0700 Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class. When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor. --- .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 - .../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +- 2 files changed, 330 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp index 7f4c3a7b787e8..36ab581f1edbd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp @@ -11,16 +11,128 @@ #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" #include using namespace clang; using namespace ento; namespace { + +class DerefAnalysisVisitor +: public ConstStmtVisitor { + // Returns true if any of child statements return true. + bool VisitChildren(const Stmt *S) { +for (const Stmt *Child : S->children()) { + if (Child && Visit(Child)) +return true; +} +return false; + } + + bool VisitBody(const Stmt *Body) { +if (!Body) + return false; + +auto [It, IsNew] = VisitedBody.insert(Body); +if (!IsNew) // This body is recursive + return false; + +return Visit(Body); + } + +public: + DerefAnalysisVisitor(const TemplateArgumentList &ArgList, + const CXXRecordDecl *ClassDecl) +: ArgList(&ArgList) +, ClassDecl(ClassDecl) + { } + + DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) +: ClassDecl(ClassDecl) + { } + + bool HasSpecializedDelete(CXXMethodDecl *Decl) { +if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) + return VisitBody(Tmpl->getBody()); +return VisitBody(Decl->getBody()); + } + + bool VisitCallExpr(const CallExpr *CE) { +auto *Callee = CE->getCallee(); +while (auto *Expr = dyn_cast(Callee)) + Callee = Expr->getSubExpr(); +if (auto *DeclRef = dyn_cast(Callee)) { + auto *Decl = DeclRef->getDecl(); + if (auto *VD = dyn_cast(Decl)) { +if (auto *Init = VD->getInit()) { + if (auto *Lambda = dyn_cast(Init)) +return VisitBody(Lambda->getBody()); +} + } else if (auto *FD = dyn_cast(Decl)) +return VisitBody(FD->getBody()); +} +return false; + } + + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { +auto *Callee = MCE->getMethodDecl(); +if (!Callee) + return false; +return VisitBody(Callee->getBody()); + } + + bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) { +auto *Arg = E->getArgument(); +while (Arg) { + if (auto *Paren = dyn_cast(Arg)) +Arg = Paren->getSubExpr(); + else if (auto *Cast = dyn_cast(Arg)) { +Arg = Cast->getSubExpr(); +auto CastType = Cast->getType(); +if (auto *PtrType = dyn_cast(CastType)) { + auto PointeeType = PtrType->getPointeeType(); + while (auto *ET = dyn_cast(PointeeType)) { +if (ET->isSugared()) + PointeeType = ET->desugar(); + } + if (auto *ParmType = dyn_cast(PointeeType)) { +if (ArgList) { + auto ParmIndex = ParmType->getIndex(); + auto Type = ArgList->get(ParmIndex).getAsType(); + if (auto *RD = dyn_cast(Type)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else if (auto *RD = dyn_cast(PointeeType)) { +if (RD->getDecl() == ClassDecl) + return true; + } +} + } else +break; +} +return false; + } + + bool VisitStmt(const Stmt *S) { return VisitChildren(S); } + + // Return false since the contents of lambda isn't necessarily executed. + // If it is executed, VisitCallExpr above will visit its body. + bool VisitLambdaExpr(const LambdaExpr *) { return false; } + +private: + const TemplateArgumentList* ArgList { null
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From e40017a2750ee39bfd1a87b5ddea620076bc4419 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/5] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 5c797d5233089..2a4da9eeaee6d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -517,11 +517,9 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; @@ -535,12 +533,14 @@ bool TrivialFunctionAnalysis::isTrivialImpl( } const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 96986631726fe..18af9e17f78b0 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -231,6 +231,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -498,6 +500,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From 5d7a259c0209a8cbb70c2718518905669db2a885 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/5] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 2a4da9eeaee6d..449e56be9984d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -535,7 +535,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } bool Result = V.Visit(Body); >From 9971dcdebc160acb708b548bf67ef5efa76d4fe0 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/5] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 69 +++ .../Checkers/WebKit/uncounted-obj-arg.
[clang] [analyzer] Check C++ base or member initializer in WebKit checkers. (PR #92220)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/92220 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check C++ base or member initializer in WebKit checkers. (PR #92220)
rniwa wrote: Thanks for the review! https://github.com/llvm/llvm-project/pull/92220 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check C++ base or member initializer in WebKit checkers. (PR #92220)
@@ -525,11 +525,19 @@ bool TrivialFunctionAnalysis::isTrivialImpl( if (!IsNew) return It->second; + TrivialFunctionAnalysisVisitor V(Cache); + + if (auto *CtorDecl = dyn_cast(D)) { +for (auto *CtorInit : CtorDecl->inits()) { + if (!V.Visit(CtorInit->getInit())) rniwa wrote: Doesn't seem to hit any crashes as far as building WebKit goes :) https://github.com/llvm/llvm-project/pull/92220 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check C++ base or member initializer in WebKit checkers. (PR #92220)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92220 None >From 9af90808426aba8086e00ed75e7753036c002c78 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 14 May 2024 22:55:36 -0700 Subject: [PATCH] [analyzer] Check C++ base or member initializer in WebKit checkers. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 10 - .../Checkers/WebKit/uncounted-obj-arg.cpp | 21 +++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 950d35a090a3f..5c797d5233089 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -525,11 +525,19 @@ bool TrivialFunctionAnalysis::isTrivialImpl( if (!IsNew) return It->second; + TrivialFunctionAnalysisVisitor V(Cache); + + if (auto *CtorDecl = dyn_cast(D)) { +for (auto *CtorInit : CtorDecl->inits()) { + if (!V.Visit(CtorInit->getInit())) +return false; +} + } + const Stmt *Body = D->getBody(); if (!Body) return false; - TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); if (Result) Cache[D] = true; diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index ed37671df3d3e..96986631726fe 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -159,10 +159,13 @@ template class OptionSet { StorageType m_storage { 0 }; }; +int atoi(const char* str); + class Number { public: Number(int v) : v(v) { } Number(double); + Number(const char* str) : v(atoi(str)) { } Number operator+(const Number&); Number& operator++() { ++v; return *this; } Number operator++(int) { Number returnValue(v); ++v; return returnValue; } @@ -173,9 +176,16 @@ class Number { int v; }; +class DerivedNumber : public Number { +public: + DerivedNumber(char c) : Number(c - '0') { } + DerivedNumber(const char* str) : Number(atoi(str)) { } +}; + class ComplexNumber { public: ComplexNumber() : realPart(0), complexPart(0) { } + ComplexNumber(int real, const char* str) : realPart(real), complexPart(str) { } ComplexNumber(const ComplexNumber&); ComplexNumber& operator++() { realPart.someMethod(); return *this; } ComplexNumber operator++(int); @@ -311,6 +321,7 @@ class RefCounted { return; } unsigned trivial60() { return ObjectWithNonTrivialDestructor { 5 }.value(); } + unsigned trivial61() { return DerivedNumber('7').value(); } static RefCounted& singleton() { static RefCounted s_RefCounted; @@ -391,6 +402,9 @@ class RefCounted { ComplexNumber nonTrivial18() { return +complex; } ComplexNumber* nonTrivial19() { return new ComplexNumber(complex); } unsigned nonTrivial20() { return ObjectWithMutatingDestructor { 7 }.value(); } + unsigned nonTrivial21() { return Number("123").value(); } + unsigned nonTrivial22() { return ComplexNumber(123, "456").real().value(); } + unsigned nonTrivial23() { return DerivedNumber("123").value(); } static unsigned s_v; unsigned v { 0 }; @@ -479,6 +493,7 @@ class UnrelatedClass { getFieldTrivial().trivial58(); // no-warning getFieldTrivial().trivial59(); // no-warning getFieldTrivial().trivial60(); // no-warning +getFieldTrivial().trivial61(); // no-warning RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning @@ -525,6 +540,12 @@ class UnrelatedClass { // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial20(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} +getFieldTrivial().nonTrivial21(); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} +getFieldTrivial().nonTrivial22(); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} +getFieldTrivial().nonTrivial23(); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} } }; ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. (PR #91873)
https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/91873 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. (PR #91873)
rniwa wrote: Thanks for the review! https://github.com/llvm/llvm-project/pull/91873 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. (PR #91873)
@@ -445,6 +456,10 @@ class TrivialFunctionAnalysisVisitor return Visit(VMT->getSubExpr()); } + bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr* BTE) { +return Visit(BTE->getSubExpr()); rniwa wrote: Oh! I guess we need to do that in `TrivialFunctionAnalysis::isTrivialImpl`? Will follow up. https://github.com/llvm/llvm-project/pull/91873 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/6] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/6] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index dd930ea4b4ab5..83a0300c627e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } TrivialFunctionAnalysisVisitor V(Cache); >From 90cf7dde6e71b1d11ee0d88cd8afe0bce47531e1 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/6] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 51 +++ .../Checkers/WebKit/uncounted-obj-arg.cpp | 20
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/5] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/5] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index dd930ea4b4ab5..83a0300c627e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } TrivialFunctionAnalysisVisitor V(Cache); >From 90cf7dde6e71b1d11ee0d88cd8afe0bce47531e1 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/5] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 51 +++ .../Checkers/WebKit/uncounted-obj-arg.cpp | 20
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/4] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/4] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index dd930ea4b4ab5..83a0300c627e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } TrivialFunctionAnalysisVisitor V(Cache); >From 90cf7dde6e71b1d11ee0d88cd8afe0bce47531e1 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/4] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 51 +++ .../Checkers/WebKit/uncounted-obj-arg.cpp | 20
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/3] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/3] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index dd930ea4b4ab5..83a0300c627e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } TrivialFunctionAnalysisVisitor V(Cache); >From 90cf7dde6e71b1d11ee0d88cd8afe0bce47531e1 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sun, 12 May 2024 15:15:33 -0700 Subject: [PATCH 3/3] Fix the TrivialFunctionAnalysis::isTrivialImpl for mutually recursive functions. Instead of assuming every function to be initially trivial, we explicitly track the set of functions that we're currently visting. When one of the currently visited function is determined to be not trivial, we clear this set to signal that all mutually recursive functions are non-trivial. We conclude that a function is trivial when Visit() call on the function body returned true **AND** the set still contains the function. To implement this new algorithm, a new public function, IsFunctionTrivial, is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in TrivialFunctionAnalysisVisitor has been updated to use this function instead of TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 51 +++ .../Checkers/WebKit/uncounted-obj-arg.cpp | 20
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
rniwa wrote: > You should add a test for mutually recursive functions. I suspect something > like this doesn't work: > > ```c++ > int non_trivial(); > int f(bool b) { return g(!b) + non_trivial(); } > int g(bool b) { return b ? f(b) : 1; } > > getFieldTrivial().f(true); // expected-warning {{...}} > getFieldTrivial().g(true); // expected-warning {{...}} > ``` Duh, of course. Thanks for pointing out the obvious flaw. https://github.com/llvm/llvm-project/pull/91876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH 1/2] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); >From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:24:10 -0700 Subject: [PATCH 2/2] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index dd930ea4b4ab5..83a0300c627e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *Body = D->getBody(); if (!Body) { Cache[D] = false; -return false; +return false; } TrivialFunctionAnalysisVisitor V(Cache); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
https://github.com/rniwa edited https://github.com/llvm/llvm-project/pull/91876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recurisve functions to be trivial. (PR #91876)
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876 >From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH] [analyzer] Allow recursive functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Allow recurisve functions to be trivial. (PR #91876)
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/91876 None >From aac9ea105506ff933d773337f3260f7770a2c5e6 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Sat, 11 May 2024 20:18:52 -0700 Subject: [PATCH] [analyzer] Allow recurisve functions to be trivial. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +- .../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ad493587affa0..dd930ea4b4ab5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - // If the function isn't in the cache, conservatively assume that - // it's not trivial until analysis completes. This makes every recursive - // function non-trivial. This also guarantees that each function - // will be scanned at most once. - auto [It, IsNew] = Cache.insert(std::make_pair(D, false)); + // Treat every recursive function as trivial until otherwise proven. + // This guarantees each function is evaluated at most once. + auto [It, IsNew] = Cache.insert(std::make_pair(D, true)); if (!IsNew) return It->second; const Stmt *Body = D->getBody(); - if (!Body) -return false; + if (!Body) { +Cache[D] = false; +return false; + } TrivialFunctionAnalysisVisitor V(Cache); bool Result = V.Visit(Body); - if (Result) -Cache[D] = true; + if (!Result) +Cache[D] = false; return Result; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp index 073f3252160ee..39bc76197d204 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp @@ -181,6 +181,8 @@ class RefCounted { void method(); void someFunction(); int otherFunction(); + unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); } + unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); } int trivial1() { return 123; } float trivial2() { return 0.3; } @@ -417,6 +419,10 @@ class UnrelatedClass { RefCounted::singleton().trivial18(); // no-warning RefCounted::singleton().someFunction(); // no-warning +getFieldTrivial().recursiveFunction(7); // no-warning +getFieldTrivial().recursiveComplexFunction(9); +// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + getFieldTrivial().someFunction(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} getFieldTrivial().nonTrivial1(); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits