[analyzer][NFC] Factor out NoOwnershipChangeVisitor

[Szelethus]

In preparation for adding essentially the same visitor to StreamChecker, this patch factors this visitor out to a common header.

I'll be the first to admit that the interface of these classes are not terrific, but it rather tightly held back by its main technical debt, which is NoStoreFuncVisitor, the main descendant of NoStateChangeVisitor.

Change-Id: I99d73ccd93a18dd145bbbc83afadbb432dd42b90

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Kristóf Umann (Szelethus)

Changes

In preparation for adding essentially the same visitor to StreamChecker, this patch factors this visitor out to a common header.

I'll be the first to admit that the interface of these classes are not terrific, but it rather tightly held back by its main technical debt, which is NoStoreFuncVisitor, the main descendant of NoStateChangeVisitor.

Change-Id: I99d73ccd93a18dd145bbbc83afadbb432dd42b90

---

Full diff: https://github.com/llvm/llvm-project/pull/94357.diff

4 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (+1)
• (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+19-128)
• (added) clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp (+116)
• (added) clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h (+78)

diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..2520119bffcdf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -78,6 +78,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NonnullGlobalConstantsChecker.cpp
+  NoOwnershipChangeVisitor.cpp
   NullabilityChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 79ab05f2c7866..41dd9cf3e6831 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -46,6 +46,7 @@
 
 #include "AllocationState.h"
 #include "InterCheckerAPI.h"
+#include "NoOwnershipChangeVisitor.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
@@ -78,13 +79,11 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetOperations.h"
-#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
-#include <climits>
 #include <functional>
 #include <optional>
 #include <utility>
@@ -401,7 +400,7 @@ class MallocChecker
   bool isFreeingCall(const CallEvent &Call) const;
   static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func);
 
-  friend class NoOwnershipChangeVisitor;
+  friend class NoMemOwnershipChangeVisitor;
 
   CallDescriptionMap<CheckFn> AllocatingMemFnMap{
       {{{"alloca"}, 1}, &MallocChecker::checkAlloca},
@@ -730,60 +729,8 @@ class MallocChecker
 //===----------------------------------------------------------------------===//
 
 namespace {
-class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
-  // The symbol whose (lack of) ownership change we are interested in.
-  SymbolRef Sym;
-  const MallocChecker &Checker;
-  using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
-
-  // Collect which entities point to the allocated memory, and could be
-  // responsible for deallocating it.
-  class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
-    SymbolRef Sym;
-    OwnerSet &Owners;
-
-  public:
-    OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
-        : Sym(Sym), Owners(Owners) {}
-
-    bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
-                       SVal Val) override {
-      if (Val.getAsSymbol() == Sym)
-        Owners.insert(Region);
-      return true;
-    }
-
-    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
-    LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const {
-      out << "Owners: {\n";
-      for (const MemRegion *Owner : Owners) {
-        out << "  ";
-        Owner->dumpToStream(out);
-        out << ",\n";
-      }
-      out << "}\n";
-    }
-  };
-
+class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor {
 protected:
-  OwnerSet getOwnersAtNode(const ExplodedNode *N) {
-    OwnerSet Ret;
-
-    ProgramStateRef State = N->getState();
-    OwnershipBindingsHandler Handler{Sym, Ret};
-    State->getStateManager().getStoreManager().iterBindings(State->getStore(),
-                                                            Handler);
-    return Ret;
-  }
-
-  LLVM_DUMP_METHOD static std::string
-  getFunctionName(const ExplodedNode *CallEnterN) {
-    if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
-            CallEnterN->getLocationAs<CallEnter>()->getCallExpr()))
-      if (const FunctionDecl *FD = CE->getDirectCallee())
-        return FD->getQualifiedNameAsString();
-    return "";
-  }
 
   /// Syntactically checks whether the callee is a deallocating function. Since
   /// we have no path-sensitive information on this call (we would need a
@@ -793,8 +740,9 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
   /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in
   /// clang/test/Analysis/NewDeleteLeaks.cpp.
   bool isFreeingCallAsWritten(const CallExpr &Call) const {
-    if (Checker.FreeingMemFnMap.lookupAsWritten(Call) ||
-        Checker.ReallocatingMemFnMap.lookupAsWritten(Call))
+    const auto *MallocChk = static_cast<const MallocChecker *>(&Checker);
+    if (MallocChk->FreeingMemFnMap.lookupAsWritten(Call) ||
+        MallocChk->ReallocatingMemFnMap.lookupAsWritten(Call))
       return true;
 
     if (const auto *Func =
@@ -804,23 +752,21 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
     return false;
   }
 
+  bool hasResourceStateChanged(ProgramStateRef CallEnterState,
+                               ProgramStateRef CallExitEndState) final {
+    return CallEnterState->get<RegionState>(Sym) !=
+           CallExitEndState->get<RegionState>(Sym);
+  }
+
   /// Heuristically guess whether the callee intended to free memory. This is
   /// done syntactically, because we are trying to argue about alternative
   /// paths of execution, and as a consequence we don't have path-sensitive
   /// information.
-  bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
+  bool doesFnIntendToHandleOwnership(const Decl *Callee,
+                                     ASTContext &ACtx) final {
     using namespace clang::ast_matchers;
     const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
 
-    // Given that the stack frame was entered, the body should always be
-    // theoretically obtainable. In case of body farms, the synthesized body
-    // is not attached to declaration, thus triggering the '!FD->hasBody()'
-    // branch. That said, would a synthesized body ever intend to handle
-    // ownership? As of today they don't. And if they did, how would we
-    // put notes inside it, given that it doesn't match any source locations?
-    if (!FD || !FD->hasBody())
-      return false;
-
     auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
                                             callExpr().bind("call")))),
                          *FD->getBody(), ACtx);
@@ -838,30 +784,7 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
     return false;
   }
 
-  bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
-                             const ExplodedNode *CallExitEndN) override {
-    if (!doesFnIntendToHandleOwnership(
-            CallExitEndN->getFirstPred()->getLocationContext()->getDecl(),
-            CallExitEndN->getState()->getAnalysisManager().getASTContext()))
-      return true;
-
-    if (CallEnterN->getState()->get<RegionState>(Sym) !=
-        CallExitEndN->getState()->get<RegionState>(Sym))
-      return true;
-
-    OwnerSet CurrOwners = getOwnersAtNode(CallEnterN);
-    OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN);
-
-    // Owners in the current set may be purged from the analyzer later on.
-    // If a variable is dead (is not referenced directly or indirectly after
-    // some point), it will be removed from the Store before the end of its
-    // actual lifetime.
-    // This means that if the ownership status didn't change, CurrOwners
-    // must be a superset of, but not necessarily equal to ExitOwners.
-    return !llvm::set_is_subset(ExitOwners, CurrOwners);
-  }
-
-  static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) {
+  PathDiagnosticPieceRef emitNote(const ExplodedNode *N) final {
     PathDiagnosticLocation L = PathDiagnosticLocation::create(
         N->getLocation(),
         N->getState()->getStateManager().getContext().getSourceManager());
@@ -870,42 +793,10 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
            "later deallocation");
   }
 
-  PathDiagnosticPieceRef
-  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
-                           const ObjCMethodCall &Call,
-                           const ExplodedNode *N) override {
-    // TODO: Implement.
-    return nullptr;
-  }
-
-  PathDiagnosticPieceRef
-  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
-                          const CXXConstructorCall &Call,
-                          const ExplodedNode *N) override {
-    // TODO: Implement.
-    return nullptr;
-  }
-
-  PathDiagnosticPieceRef
-  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
-                             const ExplodedNode *N) override {
-    // TODO: Factor the logic of "what constitutes as an entity being passed
-    // into a function call" out by reusing the code in
-    // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
-    // the printing technology in UninitializedObject's FieldChainInfo.
-    ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
-    for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
-      SVal V = Call.getArgSVal(I);
-      if (V.getAsSymbol() == Sym)
-        return emitNote(N);
-    }
-    return nullptr;
-  }
-
 public:
-  NoOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker)
-      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym),
-        Checker(*Checker) {}
+  NoMemOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker)
+      : NoOwnershipChangeVisitor(Sym, Checker) {}
+
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     static int Tag = 0;
@@ -2790,7 +2681,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
   R->markInteresting(Sym);
   R->addVisitor<MallocBugVisitor>(Sym, true);
   if (ShouldRegisterNoOwnershipChangeVisitor)
-    R->addVisitor<NoOwnershipChangeVisitor>(Sym, this);
+    R->addVisitor<NoMemOwnershipChangeVisitor>(Sym, this);
   C.emitReport(std::move(R));
 }
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
new file mode 100644
index 0000000000000..2ff76679b5ebf
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
@@ -0,0 +1,116 @@
+//===--------------------------------------------------------------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NoOwnershipChangeVisitor.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "llvm/ADT/SetOperations.h"
+
+using namespace clang;
+using namespace ento;
+using OwnerSet = NoOwnershipChangeVisitor::OwnerSet;
+
+// Collect which entities point to the allocated memory, and could be
+// responsible for deallocating it.
+class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
+  SymbolRef Sym;
+  OwnerSet &Owners;
+
+public:
+  OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
+      : Sym(Sym), Owners(Owners) {}
+
+  bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
+                     SVal Val) override {
+    if (Val.getAsSymbol() == Sym)
+      Owners.insert(Region);
+    return true;
+  }
+
+  LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
+  LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const {
+    out << "Owners: {\n";
+    for (const MemRegion *Owner : Owners) {
+      out << "  ";
+      Owner->dumpToStream(out);
+      out << ",\n";
+    }
+    out << "}\n";
+  }
+};
+
+OwnerSet NoOwnershipChangeVisitor::getOwnersAtNode(const ExplodedNode *N) {
+  OwnerSet Ret;
+
+  ProgramStateRef State = N->getState();
+  OwnershipBindingsHandler Handler{Sym, Ret};
+  State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+                                                          Handler);
+  return Ret;
+}
+
+LLVM_DUMP_METHOD std::string
+NoOwnershipChangeVisitor::getFunctionName(const ExplodedNode *CallEnterN) {
+  if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
+          CallEnterN->getLocationAs<CallEnter>()->getCallExpr()))
+    if (const FunctionDecl *FD = CE->getDirectCallee())
+      return FD->getQualifiedNameAsString();
+  return "";
+}
+
+bool NoOwnershipChangeVisitor::wasModifiedInFunction(
+    const ExplodedNode *CallEnterN, const ExplodedNode *CallExitEndN) {
+  const Decl *Callee =
+      CallExitEndN->getFirstPred()->getLocationContext()->getDecl();
+  const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+
+  // Given that the stack frame was entered, the body should always be
+  // theoretically obtainable. In case of body farms, the synthesized body
+  // is not attached to declaration, thus triggering the '!FD->hasBody()'
+  // branch. That said, would a synthesized body ever intend to handle
+  // ownership? As of today they don't. And if they did, how would we
+  // put notes inside it, given that it doesn't match any source locations?
+  if (!FD || !FD->hasBody())
+    return false;
+  if (!doesFnIntendToHandleOwnership(
+          Callee,
+          CallExitEndN->getState()->getAnalysisManager().getASTContext()))
+    return true;
+
+  if (hasResourceStateChanged(CallEnterN->getState(), CallExitEndN->getState()))
+    return true;
+
+  OwnerSet CurrOwners = getOwnersAtNode(CallEnterN);
+  OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN);
+
+  // Owners in the current set may be purged from the analyzer later on.
+  // If a variable is dead (is not referenced directly or indirectly after
+  // some point), it will be removed from the Store before the end of its
+  // actual lifetime.
+  // This means that if the ownership status didn't change, CurrOwners
+  // must be a superset of, but not necessarily equal to ExitOwners.
+  return !llvm::set_is_subset(ExitOwners, CurrOwners);
+}
+
+PathDiagnosticPieceRef NoOwnershipChangeVisitor::maybeEmitNoteForParameters(
+    PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N) {
+  // TODO: Factor the logic of "what constitutes as an entity being passed
+  // into a function call" out by reusing the code in
+  // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
+  // the printing technology in UninitializedObject's FieldChainInfo.
+  ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+  for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+    SVal V = Call.getArgSVal(I);
+    if (V.getAsSymbol() == Sym)
+      return emitNote(N);
+  }
+  return nullptr;
+}
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
new file mode 100644
index 0000000000000..04c1cd80ffc15
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h
@@ -0,0 +1,78 @@
+//===--------------------------------------------------------------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+
+namespace clang {
+namespace ento {
+
+class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor {
+protected:
+  // The symbol whose (lack of) ownership change we are interested in.
+  SymbolRef Sym;
+  const CheckerBase &Checker;
+
+  LLVM_DUMP_METHOD static std::string
+  getFunctionName(const ExplodedNode *CallEnterN);
+
+  /// Heuristically guess whether the callee intended to free the resource. This
+  /// is done syntactically, because we are trying to argue about alternative
+  /// paths of execution, and as a consequence we don't have path-sensitive
+  /// information.
+  virtual bool doesFnIntendToHandleOwnership(const Decl *Callee,
+                                             ASTContext &ACtx) = 0;
+
+  virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState,
+                                       ProgramStateRef CallExitEndState) = 0;
+
+  bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                             const ExplodedNode *CallExitEndN) final;
+
+  virtual PathDiagnosticPieceRef emitNote(const ExplodedNode *N) = 0;
+
+  PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) final {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) final {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  // Set this to final, effectively dispatch to emitNote.
+  PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) final;
+public:
+  using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
+
+private:
+  OwnerSet getOwnersAtNode(const ExplodedNode *N);
+
+public:
+  NoOwnershipChangeVisitor(SymbolRef Sym, const CheckerBase *Checker)
+      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym),
+        Checker(*Checker) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+    ID.AddPointer(Sym);
+  }
+};
+} // namespace ento
+} // namespace clang

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[steakhal]

Is this NFC?

[Szelethus]

Yes, it is, sorry about that :)

[NagyDonat]

Perfectly normal NFC change, feel free to merge it (assuming that the tests pass).

[balazske]

This class can be in an anonymous namespace?

[Szelethus]

Good catch, I'll fix that in a followup patch.

[mikerice1969]

Following up on static verifier hits. This dyn_cast result is now not checked before it is dereferenced below (FD->getBody()). Should this be a cast instead? Or should there be a check before the dereference?

[Szelethus]

Nice catch! Fix is here: #100719