Szelethus created this revision.
Szelethus added reviewers: NoQ, martong, steakhal, balazske, ASDenysPetrov,
vsavchenko.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun,
whisperity, yaxunl.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.
D105819 <https://reviews.llvm.org/D105819> Added NoOwnershipChangeVisitor, but
it is only registered when an off-by-default, hidden checker option was
enabled. The reason behind this was that it grossly overestimated the set of
functions that really needed a note:
std::string getTrainName(const Train *T) {
return T->name;
} // note: Retuning without changing the ownership of or deallocating memory
// Umm... I mean duh? Nor would I expect this function to do anything like
that...
void foo() {
Train *T = new Train("Land Plane");
print(getTrainName(T)); // note: calling getTrainName / returning from
getTrainName
} // warn: Memory leak
This patch adds a heuristic that guesses that any function that has an explicit
`operator delete` call could have be responsible for deallocating the memory
that ended up leaking. This is waaaay too conservative (see the TODOs in the
new function), but it safer to err on the side of too little than too much, and
would allow us to enable the option by default *now*, and add refinements
one-by-one.
Repository:
rG LLVM Github Monorepo
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
@@ -116,7 +116,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: widen-loops = 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 indent 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,7 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/ProgramPoint.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceManager.h"
@@ -737,6 +738,7 @@
// Definition of NoOwnershipChangeVisitor.
//===----------------------------------------------------------------------===//
+#include "clang/ASTMatchers/ASTMatchFinder.h"
namespace {
class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
SymbolRef Sym;
@@ -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 *CurrN,
const ExplodedNode *CallExitEndN) override {
+ if (!doesFnIntendToHandleOwnership(
+ CallExitEndN->getFirstPred()->getLocationContext()->getDecl(),
+ CurrN->getState()->getAnalysisManager().getASTContext()))
+ return true;
+
if (CurrN->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
