NoQ updated this revision to Diff 214441.
NoQ added a comment.
- Fix checker names for consumers that display them (eg., clang-tidy). Add a
test.
- Change bug descriptions: "Call to virtual function during construction or
destruction" -> "Pure virtual method call" | "Unexpected loss of virtual
dispatch"
- Remove the suggestion to explicitly qualify the call. It is incorrect for
nested stack frames, as it may affect the behavior when the same function is
called from elsewhere.
- Make the category reusable, fix capitalization to stay similar to other bug
categories.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65180/new/
https://reviews.llvm.org/D65180
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
clang/test/Analysis/virtualcall-plist.cpp
clang/test/Analysis/virtualcall.cpp
clang/test/Analysis/virtualcall.h
Index: clang/test/Analysis/virtualcall.h
===================================================================
--- clang/test/Analysis/virtualcall.h
+++ clang/test/Analysis/virtualcall.h
@@ -2,7 +2,7 @@
class Z {
public:
Z() {
- foo(); // impure-warning {{Call to virtual function during construction}}
+ foo(); // impure-warning {{Call to virtual method 'Z::foo' during construction bypasses virtual dispatch}}
}
virtual int foo();
};
Index: clang/test/Analysis/virtualcall.cpp
===================================================================
--- clang/test/Analysis/virtualcall.cpp
+++ clang/test/Analysis/virtualcall.cpp
@@ -1,26 +1,33 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -std=c++11 -verify=expected,impure %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -std=c++11 -verify=expected -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
// RUN: -analyzer-config \
// RUN: optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -std=c++11 -verify=expected %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
// RUN: -analyzer-checker=optin.cplusplus.VirtualCall \
+// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -std=c++11 -verify=expected,impure -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
// RUN: -analyzer-checker=optin.cplusplus.VirtualCall \
// RUN: -analyzer-config \
// RUN: optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -std=c++11 -verify=expected %s
#include "virtualcall.h"
+void clang_analyzer_warnIfReached();
+
class A {
public:
A();
@@ -30,31 +37,32 @@
virtual int foo() = 0;
virtual void bar() = 0;
void f() {
- foo(); // expected-warning{{Call to pure virtual function during construction}}
+ foo(); // expected-warning{{Call to pure virtual method 'A::foo' during construction has undefined behavior}}
+ clang_analyzer_warnIfReached(); // no-warning
}
};
-class B : public A {
+A::A() {
+ f();
+}
+
+class B {
public:
B() {
- foo(); // impure-warning {{Call to virtual function during construction}}
+ foo(); // impure-warning {{Call to virtual method 'B::foo' during construction bypasses virtual dispatch}}
}
~B();
virtual int foo();
virtual void bar() {
- foo(); // impure-warning{{Call to virtual function during destruction}}
+ foo(); // impure-warning {{Call to virtual method 'B::foo' during destruction bypasses virtual dispatch}}
}
};
-A::A() {
- f();
-}
-
B::~B() {
this->B::foo(); // no-warning
this->B::bar();
- this->foo(); // impure-warning {{Call to virtual function during destruction}}
+ this->foo(); // impure-warning {{Call to virtual method 'B::foo' during destruction bypasses virtual dispatch}}
}
class C : public B {
@@ -67,7 +75,7 @@
};
C::C() {
- f(foo()); // impure-warning {{Call to virtual function during construction}}
+ f(foo()); // impure-warning {{Call to virtual method 'C::foo' during construction bypasses virtual dispatch}}
}
class D : public B {
@@ -121,23 +129,23 @@
G g;
g.foo();
g.bar(); // no warning
- f(); // impure-warning {{Call to virtual function during construction}}
+ f(); // impure-warning {{Call to virtual method 'H::f' during construction bypasses virtual dispatch}}
H &h = *this;
- h.f(); // impure-warning {{Call to virtual function during construction}}
+ h.f(); // impure-warning {{Call to virtual method 'H::f' during construction bypasses virtual dispatch}}
}
};
class X {
public:
X() {
- g(); // impure-warning {{Call to virtual function during construction}}
+ g(); // impure-warning {{Call to virtual method 'X::g' during construction bypasses virtual dispatch}}
}
X(int i) {
if (i > 0) {
X x(i - 1);
x.g(); // no warning
}
- g(); // impure-warning {{Call to virtual function during construction}}
+ g(); // impure-warning {{Call to virtual method 'X::g' during construction bypasses virtual dispatch}}
}
virtual void g();
};
@@ -158,7 +166,7 @@
virtual void foo();
};
void N::callFooOfM(M *m) {
- m->foo(); // impure-warning {{Call to virtual function during construction}}
+ m->foo(); // impure-warning {{Call to virtual method 'M::foo' during construction bypasses virtual dispatch}}
}
class Y {
@@ -166,7 +174,7 @@
virtual void foobar();
void fooY() {
F f1;
- foobar(); // impure-warning {{Call to virtual function during construction}}
+ foobar(); // impure-warning {{Call to virtual method 'Y::foobar' during construction bypasses virtual dispatch}}
}
Y() { fooY(); }
};
Index: clang/test/Analysis/virtualcall-plist.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/virtualcall-plist.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus \
+// RUN: -analyzer-output=plist -o %t.plist -w -verify %s
+// RUN: cat %t.plist | FileCheck %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,optin.cplusplus \
+// RUN: -analyzer-output=plist -o %t.plist -w -verify %s
+// RUN: cat %t.plist | FileCheck %s
+
+struct S {
+ virtual void foo();
+ virtual void bar() = 0;
+
+ S() {
+ // CHECK: Call to virtual method 'S::foo' during construction bypasses virtual dispatch
+ // CHECK: optin.cplusplus.VirtualCall
+ foo(); // expected-warning{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+ // CHECK: Call to pure virtual method 'S::bar' during construction has undefined behavior
+ // CHECK: cplusplus.PureVirtualCall
+ bar(); // expected-warning{{Call to pure virtual method 'S::bar' during construction has undefined behavior}}
+ }
+};
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -17,4 +17,5 @@
"Memory (Core Foundation/Objective-C/OSObject)";
const char * const MemoryError = "Memory error";
const char * const UnixAPI = "Unix API";
+const char * const CXXObjectLifecycle = "C++ object lifecycle";
}}}
Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -6,7 +6,7 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines a checker that checks virtual function calls during
+// This file defines a checker that checks virtual method calls during
// construction or destruction of C++ objects.
//
//===----------------------------------------------------------------------===//
@@ -40,11 +40,9 @@
namespace {
class VirtualCallChecker
: public Checker<check::BeginFunction, check::EndFunction, check::PreCall> {
- mutable std::unique_ptr<BugType> BT;
-
public:
- // The flag to determine if pure virtual functions should be issued only.
- bool IsPureOnly = true;
+ // These are going to be null if the respective check is disabled.
+ mutable std::unique_ptr<BugType> BT_Pure, BT_Impure;
void checkBeginFunction(CheckerContext &C) const;
void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
@@ -53,15 +51,13 @@
private:
void registerCtorDtorCallInState(bool IsBeginFunction,
CheckerContext &C) const;
- void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg,
- CheckerContext &C) const;
};
} // end namespace
// GDM (generic data map) to the memregion of this for the ctor and dtor.
REGISTER_MAP_WITH_PROGRAMSTATE(CtorDtorMap, const MemRegion *, ObjectState)
-// The function to check if a callexpr is a virtual function.
+// The function to check if a callexpr is a virtual method call.
static bool isVirtualCall(const CallExpr *CE) {
bool CallIsNonVirtual = false;
@@ -106,11 +102,9 @@
const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
if (!MD)
return;
+
ProgramStateRef State = C.getState();
const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-
- if (IsPureOnly && !MD->isPure())
- return;
if (!isVirtualCall(CE))
return;
@@ -118,29 +112,40 @@
const ObjectState *ObState = State->get<CtorDtorMap>(Reg);
if (!ObState)
return;
- // Check if a virtual method is called.
- // The GDM of constructor and destructor should be true.
- if (*ObState == ObjectState::CtorCalled) {
- if (IsPureOnly && MD->isPure())
- reportBug("Call to pure virtual function during construction", true, Reg,
- C);
- else if (!MD->isPure())
- reportBug("Call to virtual function during construction", false, Reg, C);
- else
- reportBug("Call to pure virtual function during construction", false, Reg,
- C);
- }
- if (*ObState == ObjectState::DtorCalled) {
- if (IsPureOnly && MD->isPure())
- reportBug("Call to pure virtual function during destruction", true, Reg,
- C);
- else if (!MD->isPure())
- reportBug("Call to virtual function during destruction", false, Reg, C);
- else
- reportBug("Call to pure virtual function during construction", false, Reg,
- C);
+ bool IsPure = MD->isPure();
+
+ // At this point we're sure that we're calling a virtual method
+ // during construction or destruction, so we'll emit a report.
+ SmallString<128> Msg;
+ llvm::raw_svector_ostream OS(Msg);
+ OS << "Call to ";
+ if (IsPure)
+ OS << "pure ";
+ OS << "virtual method '" << MD->getParent()->getNameAsString()
+ << "::" << MD->getNameAsString() << "' during ";
+ if (*ObState == ObjectState::CtorCalled)
+ OS << "construction ";
+ else
+ OS << "destruction ";
+ if (IsPure)
+ OS << "has undefined behavior";
+ else
+ OS << "bypasses virtual dispatch";
+
+ ExplodedNode *N =
+ IsPure ? C.generateErrorNode() : C.generateNonFatalErrorNode();
+ if (!N)
+ return;
+
+ const std::unique_ptr<BugType> &BT = IsPure ? BT_Pure : BT_Impure;
+ if (!BT) {
+ assert(BT == BT_Impure && "Pure-only check must be enabled!");
+ return;
}
+
+ auto Report = llvm::make_unique<BugReport>(*BT, OS.str(), N);
+ C.emitReport(std::move(Report));
}
void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction,
@@ -182,34 +187,21 @@
}
}
-void VirtualCallChecker::reportBug(StringRef Msg, bool IsSink,
- const MemRegion *Reg,
- CheckerContext &C) const {
- ExplodedNode *N;
- if (IsSink)
- N = C.generateErrorNode();
- else
- N = C.generateNonFatalErrorNode();
-
- if (!N)
- return;
- if (!BT)
- BT.reset(new BugType(
- this, "Call to virtual function during construction or destruction",
- "C++ Object Lifecycle"));
-
- auto Reporter = llvm::make_unique<BugReport>(*BT, Msg, N);
- C.emitReport(std::move(Reporter));
-}
-
void ento::registerPureVirtualCallChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<VirtualCallChecker>();
+ auto *Chk = Mgr.registerChecker<VirtualCallChecker>();
+ Chk->BT_Pure = llvm::make_unique<BugType>(
+ Mgr.getCurrentCheckName(), "Pure virtual method call",
+ categories::CXXObjectLifecycle);
}
void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
auto *Chk = Mgr.getChecker<VirtualCallChecker>();
- Chk->IsPureOnly = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
- Mgr.getCurrentCheckName(), "PureOnly");
+ if (!Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ Mgr.getCurrentCheckName(), "PureOnly")) {
+ Chk->BT_Impure = llvm::make_unique<BugType>(
+ Mgr.getCurrentCheckName(), "Unexpected loss of virtual dispatch",
+ categories::CXXObjectLifecycle);
+ }
}
bool ento::shouldRegisterPureVirtualCallChecker(const LangOptions &LO) {
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
@@ -18,6 +18,7 @@
extern const char * const MemoryRefCount;
extern const char * const MemoryError;
extern const char * const UnixAPI;
+ extern const char * const CXXObjectLifecycle;
}
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
