[clang] 9d69072 - [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker
Kirstóf Umann via cfe-commits
cfe-commits at lists.llvm.org
Mon May 18 15:19:06 PDT 2020
Author: Kirstóf Umann
Date: 2020-05-19T00:18:38+02:00
New Revision: 9d69072fb80755a0029a01c74892b4bf03f20f65
URL: https://github.com/llvm/llvm-project/commit/9d69072fb80755a0029a01c74892b4bf03f20f65
DIFF: https://github.com/llvm/llvm-project/commit/9d69072fb80755a0029a01c74892b4bf03f20f65.diff
LOG: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker
One of the pain points in simplifying MallocCheckers interface by gradually
changing to CallEvent is that a variety of C++ allocation and deallocation
functionalities are modeled through preStmt<...> where CallEvent is unavailable,
and a single one of these callbacks can prevent a mass parameter change.
This patch introduces a new CallEvent, CXXDeallocatorCall, which happens after
preStmt<CXXDeleteExpr>, and can completely replace that callback as
demonstrated.
Differential Revision: https://reviews.llvm.org/D75430
Added:
clang/unittests/StaticAnalyzer/CallEventTest.cpp
Modified:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
clang/unittests/StaticAnalyzer/CMakeLists.txt
Removed:
################################################################################
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index bc562a4ca6f1..871875b45b1b 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -67,8 +67,9 @@ enum CallEventKind {
CE_BEG_CXX_CONSTRUCTOR_CALLS = CE_CXXConstructor,
CE_END_CXX_CONSTRUCTOR_CALLS = CE_CXXInheritedConstructor,
CE_CXXAllocator,
+ CE_CXXDeallocator,
CE_BEG_FUNCTION_CALLS = CE_Function,
- CE_END_FUNCTION_CALLS = CE_CXXAllocator,
+ CE_END_FUNCTION_CALLS = CE_CXXDeallocator,
CE_Block,
CE_ObjCMessage
};
@@ -1045,6 +1046,55 @@ class CXXAllocatorCall : public AnyFunctionCall {
}
};
+/// Represents the memory deallocation call in a C++ delete-expression.
+///
+/// This is a call to "operator delete".
+// FIXME: CXXDeleteExpr isn't present for custom delete operators, or even for
+// some those that are in the standard library, like the no-throw or align_val
+// versions.
+// Some pointers:
+// http://lists.llvm.org/pipermail/cfe-dev/2020-April/065080.html
+// clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
+// clang/unittests/StaticAnalyzer/CallEventTest.cpp
+class CXXDeallocatorCall : public AnyFunctionCall {
+ friend class CallEventManager;
+
+protected:
+ CXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef St,
+ const LocationContext *LCtx)
+ : AnyFunctionCall(E, St, LCtx) {}
+ CXXDeallocatorCall(const CXXDeallocatorCall &Other) = default;
+
+ void cloneTo(void *Dest) const override {
+ new (Dest) CXXDeallocatorCall(*this);
+ }
+
+public:
+ virtual const CXXDeleteExpr *getOriginExpr() const {
+ return cast<CXXDeleteExpr>(AnyFunctionCall::getOriginExpr());
+ }
+
+ const FunctionDecl *getDecl() const override {
+ return getOriginExpr()->getOperatorDelete();
+ }
+
+ unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+
+ const Expr *getArgExpr(unsigned Index) const override {
+ // CXXDeleteExpr's only have a single argument.
+ return getOriginExpr()->getArgument();
+ }
+
+ Kind getKind() const override { return CE_CXXDeallocator; }
+ virtual StringRef getKindAsString() const override {
+ return "CXXDeallocatorCall";
+ }
+
+ static bool classof(const CallEvent *CE) {
+ return CE->getKind() == CE_CXXDeallocator;
+ }
+};
+
/// Represents the ways an Objective-C message send can occur.
//
// Note to maintainers: OCM_Message should always be last, since it does not
@@ -1367,6 +1417,12 @@ class CallEventManager {
const LocationContext *LCtx) {
return create<CXXAllocatorCall>(E, State, LCtx);
}
+
+ CallEventRef<CXXDeallocatorCall>
+ getCXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef State,
+ const LocationContext *LCtx) {
+ return create<CXXDeallocatorCall>(E, State, LCtx);
+ }
};
template <typename T>
diff --git a/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
index 97556ca856a0..7c5833762008 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
@@ -92,6 +92,8 @@ void DeleteWithNonVirtualDtorChecker::checkPreStmt(const CXXDeleteExpr *DE,
"Logic error"));
ExplodedNode *N = C.generateNonFatalErrorNode();
+ if (!N)
+ return;
auto R = std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
// Mark region of problematic base class for later use in the BugVisitor.
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 52786bcaf072..2bff317873c1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -284,8 +284,8 @@ class MallocChecker
check::ConstPointerEscape, check::PreStmt<ReturnStmt>,
check::EndFunction, check::PreCall, check::PostCall,
check::PostStmt<CXXNewExpr>, check::NewAllocator,
- check::PreStmt<CXXDeleteExpr>, check::PostStmt<BlockExpr>,
- check::PostObjCMessage, check::Location, eval::Assume> {
+ check::PostStmt<BlockExpr>, check::PostObjCMessage,
+ check::Location, eval::Assume> {
public:
/// In pessimistic mode, the checker assumes that it does not know which
/// functions might free the memory.
@@ -317,7 +317,6 @@ class MallocChecker
void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
CheckerContext &C) const;
- void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
@@ -1412,25 +1411,6 @@ ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
return State;
}
-void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
- CheckerContext &C) const {
-
- if (!ChecksEnabled[CK_NewDeleteChecker])
- if (SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol())
- checkUseAfterFree(Sym, C, DE->getArgument());
-
- if (!isStandardNewDelete(DE->getOperatorDelete()))
- return;
-
- ProgramStateRef State = C.getState();
- bool IsKnownToBeAllocated;
- State = FreeMemAux(C, DE->getArgument(), DE, State,
- /*Hold*/ false, IsKnownToBeAllocated,
- (DE->isArrayForm() ? AF_CXXNewArray : AF_CXXNew));
-
- C.addTransition(State);
-}
-
static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) {
// If the first selector piece is one of the names below, assume that the
// object takes ownership of the memory, promising to eventually deallocate it
@@ -2620,7 +2600,27 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
void MallocChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
- if (const CXXDestructorCall *DC = dyn_cast<CXXDestructorCall>(&Call)) {
+ if (const auto *DC = dyn_cast<CXXDeallocatorCall>(&Call)) {
+ const CXXDeleteExpr *DE = DC->getOriginExpr();
+
+ if (!ChecksEnabled[CK_NewDeleteChecker])
+ if (SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol())
+ checkUseAfterFree(Sym, C, DE->getArgument());
+
+ if (!isStandardNewDelete(DC->getDecl()))
+ return;
+
+ ProgramStateRef State = C.getState();
+ bool IsKnownToBeAllocated;
+ State = FreeMemAux(C, DE->getArgument(), DE, State,
+ /*Hold*/ false, IsKnownToBeAllocated,
+ (DE->isArrayForm() ? AF_CXXNewArray : AF_CXXNew));
+
+ C.addTransition(State);
+ return;
+ }
+
+ if (const auto *DC = dyn_cast<CXXDestructorCall>(&Call)) {
SymbolRef Sym = DC->getCXXThisVal().getAsSymbol();
if (!Sym || checkDoubleDelete(Sym, C))
return;
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index e9a1ccdb663b..36b930faf2d0 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1649,8 +1649,10 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet PreVisit;
const auto *CDE = cast<CXXDeleteExpr>(S);
getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+ ExplodedNodeSet PostVisit;
+ getCheckerManager().runCheckersForPostStmt(PostVisit, PreVisit, S, *this);
- for (const auto i : PreVisit)
+ for (const auto i : PostVisit)
VisitCXXDeleteExpr(CDE, i, Dst);
Bldr.addNodes(Dst);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index d05b31a64427..7171dd220485 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -875,13 +875,18 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
ExplodedNode *Pred, ExplodedNodeSet &Dst) {
- StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
- ProgramStateRef state = Pred->getState();
- Bldr.generateNode(CDE, Pred, state);
+
+ CallEventManager &CEMgr = getStateManager().getCallEventManager();
+ CallEventRef<CXXDeallocatorCall> Call = CEMgr.getCXXDeallocatorCall(
+ CDE, Pred->getState(), Pred->getLocationContext());
+
+ ExplodedNodeSet DstPreCall;
+ getCheckerManager().runCheckersForPreCall(DstPreCall, Pred, *Call, *this);
+
+ getCheckerManager().runCheckersForPostCall(Dst, DstPreCall, *Call, *this);
}
-void ExprEngine::VisitCXXCatchStmt(const CXXCatchStmt *CS,
- ExplodedNode *Pred,
+void ExprEngine::VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
const VarDecl *VD = CS->getExceptionDecl();
if (!VD) {
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 781cc9f7943d..2ac0160e15c2 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,17 +10,18 @@
//
//===----------------------------------------------------------------------===//
-#include "clang/AST/Decl.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "PrettyStackTraceLocationContext.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/Analysis/Analyses/LiveVariables.h"
#include "clang/Analysis/ConstructionContext.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/Statistic.h"
+#include "llvm/Support/Compiler.h"
#include "llvm/Support/SaveAndRestore.h"
using namespace clang;
@@ -819,6 +820,8 @@ ExprEngine::mayInlineCallKind(const CallEvent &Call, const ExplodedNode *Pred,
return CIP_DisallowedOnce;
break;
}
+ case CE_CXXDeallocator:
+ LLVM_FALLTHROUGH;
case CE_CXXAllocator:
if (Opts.MayInlineCXXAllocator)
break;
diff --git a/clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp b/clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
index dfb3bbfe1bc0..f28fb4593a40 100644
--- a/clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
+++ b/clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
@@ -28,8 +28,8 @@ void f() {
// Default behavior: Calls operator delete(ptr), or operator delete(ptr,
// alignment), respectively.
- // FIXME: All calls to operator new should be CXXAllocatorCall.
- // FIXME: PostStmt<CXXDeleteExpr> should be present.
+ // FIXME: All calls to operator new should be CXXAllocatorCall, and calls to
+ // operator delete should be CXXDeallocatorCall.
{
int *p = new int;
delete p;
@@ -38,6 +38,9 @@ void f() {
// CHECK-NEXT: PreStmt<CXXNewExpr>
// CHECK-NEXT: PostStmt<CXXNewExpr>
// CHECK-NEXT: PreStmt<CXXDeleteExpr>
+ // CHECK-NEXT: PostStmt<CXXDeleteExpr>
+ // CHECK-NEXT: PreCall (operator delete) [CXXDeallocatorCall]
+ // CHECK-NEXT: PostCall (operator delete) [CXXDeallocatorCall]
p = new int;
operator delete(p, 23542368);
@@ -87,6 +90,9 @@ void f() {
// CHECK-NEXT: PreStmt<CXXNewExpr>
// CHECK-NEXT: PostStmt<CXXNewExpr>
// CHECK-NEXT: PreStmt<CXXDeleteExpr>
+ // CHECK-NEXT: PostStmt<CXXDeleteExpr>
+ // CHECK-NEXT: PreCall (operator delete[]) [CXXDeallocatorCall]
+ // CHECK-NEXT: PostCall (operator delete[]) [CXXDeallocatorCall]
p = new int[2];
operator delete[](p, 23542368);
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 5ce660f00040..1070f124921d 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(StaticAnalysisTests
AnalyzerOptionsTest.cpp
CallDescriptionTest.cpp
+ CallEventTest.cpp
StoreTest.cpp
RegisterCustomCheckersTest.cpp
SymbolReaperTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/CallEventTest.cpp b/clang/unittests/StaticAnalyzer/CallEventTest.cpp
new file mode 100644
index 000000000000..43ee7320721b
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -0,0 +1,89 @@
+//===----------------------------------------------------------------------===//
+//
+// 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/PathSensitive/CallEvent.h"
+#include "CheckerRegistration.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+void reportBug(const CheckerBase *Checker, const CallEvent &Call,
+ CheckerContext &C, StringRef WarningMsg) {
+ C.getBugReporter().EmitBasicReport(
+ nullptr, Checker, "", categories::LogicError, WarningMsg,
+ PathDiagnosticLocation(Call.getOriginExpr(), C.getSourceManager(),
+ C.getLocationContext()),
+ {});
+}
+
+class CXXDeallocatorChecker : public Checker<check::PreCall> {
+ std::unique_ptr<BuiltinBug> BT_uninitField;
+
+public:
+ CXXDeallocatorChecker()
+ : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {}
+
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+ const auto *DC = dyn_cast<CXXDeallocatorCall>(&Call);
+ if (!DC) {
+ return;
+ }
+
+ SmallString<100> WarningBuf;
+ llvm::raw_svector_ostream WarningOS(WarningBuf);
+ WarningOS << "NumArgs: " << DC->getNumArgs();
+
+ reportBug(this, *DC, C, WarningBuf);
+ }
+};
+
+void addCXXDeallocatorChecker(AnalysisASTConsumer &AnalysisConsumer,
+ AnalyzerOptions &AnOpts) {
+ AnOpts.CheckersAndPackages = {{"test.CXXDeallocator", true}};
+ AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+ Registry.addChecker<CXXDeallocatorChecker>("test.CXXDeallocator",
+ "Description", "");
+ });
+}
+
+// TODO: What we should really be testing here is all the
diff erent varieties
+// of delete operators, and wether the retrieval of their arguments works as
+// intended. At the time of writing this file, CXXDeallocatorCall doesn't pick
+// up on much of those due to the AST not containing CXXDeleteExpr for most of
+// the standard/custom deletes.
+TEST(CXXDeallocatorCall, SimpleDestructor) {
+ std::string Diags;
+ EXPECT_TRUE(runCheckerOnCode<addCXXDeallocatorChecker>(R"(
+ struct A {};
+
+ void f() {
+ A *a = new A;
+ delete a;
+ }
+ )",
+ Diags));
+ EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
More information about the cfe-commits
mailing list