[alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object references within trivial statements (#82229)

This PR makes alpha.webkit.UncountedLocalVarsChecker ignore raw
references and pointers to a ref counted type which appears within
"trival" statements. To do this, this PR extends TrivialFunctionAnalysis
so that it can also analyze "triviality" of statements as well as that
of functions Each Visit* function is now augmented with
withCachedResult, which is responsible for looking up and updating the
cache for each Visit* functions.

As this PR dramatically improves the false positive rate of the checker,
it also deletes the code to ignore raw pointers and references within if
and for statements.

Author: rniwa

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp

@@ -253,6 +253,19 @@ class TrivialFunctionAnalysisVisitor
     return true;
   }
 
+  template <typename CheckFunction>
+  bool WithCachedResult(const Stmt *S, CheckFunction Function) {
+    // If the statement isn't in the cache, conservatively assume that
+    // it's not trivial until analysis completes. Insert false to the cache
+    // first to avoid infinite recursion.
+    auto [It, IsNew] = Cache.insert(std::make_pair(S, false));
+    if (!IsNew)
+      return It->second;
+    bool Result = Function();
+    Cache[S] = Result;
+    return Result;
+  }
+
 public:
   using CacheTy = TrivialFunctionAnalysis::CacheTy;
 
@@ -267,7 +280,7 @@ class TrivialFunctionAnalysisVisitor
   bool VisitCompoundStmt(const CompoundStmt *CS) {
     // A compound statement is allowed as long each individual sub-statement
     // is trivial.
-    return VisitChildren(CS);
+    return WithCachedResult(CS, [&]() { return VisitChildren(CS); });
   }
 
   bool VisitReturnStmt(const ReturnStmt *RS) {
@@ -279,17 +292,36 @@ class TrivialFunctionAnalysisVisitor
 
   bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); }
   bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); }
-  bool VisitIfStmt(const IfStmt *IS) { return VisitChildren(IS); }
+  bool VisitIfStmt(const IfStmt *IS) {
+    return WithCachedResult(IS, [&]() { return VisitChildren(IS); });
+  }
+  bool VisitForStmt(const ForStmt *FS) {
+    return WithCachedResult(FS, [&]() { return VisitChildren(FS); });
+  }
+  bool VisitCXXForRangeStmt(const CXXForRangeStmt *FS) {
+    return WithCachedResult(FS, [&]() { return VisitChildren(FS); });
+  }
+  bool VisitWhileStmt(const WhileStmt *WS) {
+    return WithCachedResult(WS, [&]() { return VisitChildren(WS); });
+  }
   bool VisitSwitchStmt(const SwitchStmt *SS) { return VisitChildren(SS); }
   bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); }
   bool VisitDefaultStmt(const DefaultStmt *DS) { return VisitChildren(DS); }
 
   bool VisitUnaryOperator(const UnaryOperator *UO) {
     // Operator '*' and '!' are allowed as long as the operand is trivial.
-    if (UO->getOpcode() == UO_Deref || UO->getOpcode() == UO_AddrOf ||
-        UO->getOpcode() == UO_LNot)
+    auto op = UO->getOpcode();
+    if (op == UO_Deref || op == UO_AddrOf || op == UO_LNot)
       return Visit(UO->getSubExpr());
 
+    if (UO->isIncrementOp() || UO->isDecrementOp()) {
+      // Allow increment or decrement of a POD type.
+      if (auto *RefExpr = dyn_cast<DeclRefExpr>(UO->getSubExpr())) {
+        if (auto *Decl = dyn_cast<VarDecl>(RefExpr->getDecl()))
+          return Decl->isLocalVarDeclOrParm() &&
+                 Decl->getType().isPODType(Decl->getASTContext());
+      }
+    }
     // Other operators are non-trivial.
     return false;
   }
@@ -304,22 +336,6 @@ class TrivialFunctionAnalysisVisitor
     return VisitChildren(CO);
   }
 
-  bool VisitDeclRefExpr(const DeclRefExpr *DRE) {
-    if (auto *decl = DRE->getDecl()) {
-      if (isa<ParmVarDecl>(decl))
-        return true;
-      if (isa<EnumConstantDecl>(decl))
-        return true;
-      if (auto *VD = dyn_cast<VarDecl>(decl)) {
-        if (VD->hasConstantInitialization() && VD->getEvaluatedValue())
-          return true;
-        auto *Init = VD->getInit();
-        return !Init || Visit(Init);
-      }
-    }
-    return false;
-  }
-
   bool VisitAtomicExpr(const AtomicExpr *E) { return VisitChildren(E); }
 
   bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) {
@@ -436,6 +452,11 @@ class TrivialFunctionAnalysisVisitor
     return true;
   }
 
+  bool VisitDeclRefExpr(const DeclRefExpr *DRE) {
+    // The use of a variable is trivial.
+    return true;
+  }
+
   // Constant literal expressions are always trivial
   bool VisitIntegerLiteral(const IntegerLiteral *E) { return true; }
   bool VisitFloatingLiteral(const FloatingLiteral *E) { return true; }
@@ -449,7 +470,7 @@ class TrivialFunctionAnalysisVisitor
   }
 
 private:
-  CacheTy Cache;
+  CacheTy &Cache;
 };
 
 bool TrivialFunctionAnalysis::isTrivialImpl(
@@ -474,4 +495,17 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
   return Result;
 }
 
+bool TrivialFunctionAnalysis::isTrivialImpl(
+    const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) {
+  // If the statement isn't in the cache, conservatively assume that
+  // it's not trivial until analysis completes. Unlike a function case,
+  // we don't insert an entry into the cache until Visit returns
+  // since Visit* functions themselves make use of the cache.
+
+  TrivialFunctionAnalysisVisitor V(Cache);
+  bool Result = V.Visit(S);
+  assert(Cache.contains(S) && "Top-level statement not properly cached!");
+  return Result;
+}
+
 } // namespace clang

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h

@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/PointerUnion.h"
 #include <optional>
 
 namespace clang {
@@ -19,6 +20,7 @@ class CXXMethodDecl;
 class CXXRecordDecl;
 class Decl;
 class FunctionDecl;
+class Stmt;
 class Type;
 
 // Ref-countability of a type is implicitly defined by Ref<T> and RefPtr<T>
@@ -71,14 +73,17 @@ class TrivialFunctionAnalysis {
 public:
   /// \returns true if \p D is a "trivial" function.
   bool isTrivial(const Decl *D) const { return isTrivialImpl(D, TheCache); }
+  bool isTrivial(const Stmt *S) const { return isTrivialImpl(S, TheCache); }
 
 private:
   friend class TrivialFunctionAnalysisVisitor;
 
-  using CacheTy = llvm::DenseMap<const Decl *, bool>;
+  using CacheTy =
+      llvm::DenseMap<llvm::PointerUnion<const Decl *, const Stmt *>, bool>;
   mutable CacheTy TheCache{};
 
   static bool isTrivialImpl(const Decl *D, CacheTy &Cache);
+  static bool isTrivialImpl(const Stmt *S, CacheTy &Cache);
 };
 
 } // namespace clang

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

@@ -26,28 +26,6 @@ using namespace ento;
 
 namespace {
 
-// for ( int a = ...) ... true
-// for ( int a : ...) ... true
-// if ( int* a = ) ... true
-// anything else ... false
-bool isDeclaredInForOrIf(const VarDecl *Var) {
-  assert(Var);
-  auto &ASTCtx = Var->getASTContext();
-  auto parent = ASTCtx.getParents(*Var);
-
-  if (parent.size() == 1) {
-    if (auto *DS = parent.begin()->get<DeclStmt>()) {
-      DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
-      if (grandParent.size() == 1) {
-        return grandParent.begin()->get<ForStmt>() ||
-               grandParent.begin()->get<IfStmt>() ||
-               grandParent.begin()->get<CXXForRangeStmt>();
-      }
-    }
-  }
-  return false;
-}
-
 // FIXME: should be defined by anotations in the future
 bool isRefcountedStringsHack(const VarDecl *V) {
   assert(V);
@@ -143,6 +121,11 @@ class UncountedLocalVarsChecker
     // want to visit those, so we make our own RecursiveASTVisitor.
     struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
       const UncountedLocalVarsChecker *Checker;
+
+      TrivialFunctionAnalysis TFA;
+
+      using Base = RecursiveASTVisitor<LocalVisitor>;
+
       explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
           : Checker(Checker) {
         assert(Checker);
@@ -155,6 +138,36 @@ class UncountedLocalVarsChecker
         Checker->visitVarDecl(V);
         return true;
       }
+
+      bool TraverseIfStmt(IfStmt *IS) {
+        if (!TFA.isTrivial(IS))
+          return Base::TraverseIfStmt(IS);
+        return true;
+      }
+
+      bool TraverseForStmt(ForStmt *FS) {
+        if (!TFA.isTrivial(FS))
+          return Base::TraverseForStmt(FS);
+        return true;
+      }
+
+      bool TraverseCXXForRangeStmt(CXXForRangeStmt *FRS) {
+        if (!TFA.isTrivial(FRS))
+          return Base::TraverseCXXForRangeStmt(FRS);
+        return true;
+      }
+
+      bool TraverseWhileStmt(WhileStmt *WS) {
+        if (!TFA.isTrivial(WS))
+          return Base::TraverseWhileStmt(WS);
+        return true;
+      }
+
+      bool TraverseCompoundStmt(CompoundStmt *CS) {
+        if (!TFA.isTrivial(CS))
+          return Base::TraverseCompoundStmt(CS);
+        return true;
+      }
     };
 
     LocalVisitor visitor(this);
@@ -189,18 +202,16 @@ class UncountedLocalVarsChecker
                 dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
           const auto *MaybeGuardianArgType =
               MaybeGuardian->getType().getTypePtr();
-          if (!MaybeGuardianArgType)
-            return;
-          const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
-              MaybeGuardianArgType->getAsCXXRecordDecl();
-          if (!MaybeGuardianArgCXXRecord)
-            return;
-
-          if (MaybeGuardian->isLocalVarDecl() &&
-              (isRefCounted(MaybeGuardianArgCXXRecord) ||
-               isRefcountedStringsHack(MaybeGuardian)) &&
-              isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
-            return;
+          if (MaybeGuardianArgType) {
+            const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+                MaybeGuardianArgType->getAsCXXRecordDecl();
+            if (MaybeGuardianArgCXXRecord) {
+              if (MaybeGuardian->isLocalVarDecl() &&
+                  (isRefCounted(MaybeGuardianArgCXXRecord) ||
+                   isRefcountedStringsHack(MaybeGuardian)) &&
+                  isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
+                return;
+            }
           }
 
           // Parameters are guaranteed to be safe for the duration of the call
@@ -219,9 +230,6 @@ class UncountedLocalVarsChecker
     if (!V->isLocalVarDecl())
       return true;
 
-    if (isDeclaredInForOrIf(V))
-      return true;
-
     return false;
   }
 

clang/test/Analysis/Checkers/WebKit/mock-types.h

@@ -62,6 +62,8 @@ struct RefCountable {
   static Ref<RefCountable> create();
   void ref() {}
   void deref() {}
+  void method();
+  int trivial() { return 123; }
 };
 
 template <typename T> T *downcast(T *t) { return t; }