[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d359f6c7386: [analyzer] MallocChecker: Add notes from 
NoOwnershipChangeVisitor only when a… (authored by Szelethus).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108753

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp
  clang/test/Analysis/analyzer-config.c


Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -117,7 +117,7 @@
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
-// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: verbose-report-filename = false
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===================================================================
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -20,15 +20,16 @@
 
 bool coin();
 
+// TODO: AST analysis of sink would reveal that it doesn't intent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
 namespace memory_allocated_in_fn_call {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   sink(new int(5)); // expected-note {{Memory is allocated}}
-                    // ownership-note@-1 {{Calling 'sink'}}
-                    // ownership-note@-2 {{Returning from 'sink'}}
 } // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential memory leak}}
 
@@ -109,17 +110,14 @@
 
 } // namespace memory_shared_with_ptr_of_same_lifetime
 
-// TODO: We don't want a note here. sink() doesn't seem like a function that
-// even attempts to take care of any memory ownership problems.
 namespace memory_passed_into_fn_that_doesnt_intend_to_free {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   int *ptr = new int(5); // expected-note {{Memory is allocated}}
-  sink(ptr);             // ownership-note {{Calling 'sink'}}
-                         // ownership-note@-1 {{Returning from 'sink'}}
+  sink(ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -52,6 +52,8 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
@@ -791,9 +793,28 @@
     return "";
   }
 
+  bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
+    using namespace clang::ast_matchers;
+    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+    if (!FD)
+      return false;
+    // TODO: Operator delete is hardly the only deallocator -- Can we reuse
+    // isFreeingCall() or something thats already here?
+    auto Deallocations = match(
+        stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
+             ), *FD->getBody(), ACtx);
+    // TODO: Ownership my change with an attempt to store the allocated memory.
+    return !Deallocations.empty();
+  }
+
   virtual 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;
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -493,8 +493,8 @@
                   "that neither deallocated it, or have taken responsibility "
                   "of the ownership are noted, similarly to "
                   "NoStoreFuncVisitor.",
-                  "false",
-                  InAlpha,
+                  "true",
+                  Released,
                   Hide>
   ]>,
   Dependencies<[CStringModeling]>,

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits