r289309 - [analyzer] Improve VirtualCallChecker diagnostics and move into optin package.
Devin Coughlin via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 9 17:16:10 PST 2016
Author: dcoughlin
Date: Fri Dec 9 19:16:09 2016
New Revision: 289309
URL: http://llvm.org/viewvc/llvm-project?rev=289309&view=rev
Log:
[analyzer] Improve VirtualCallChecker diagnostics and move into optin package.
The VirtualCallChecker is in alpha because its interprocedural diagnostics
represent the call path textually in the diagnostic message rather than with a
path sensitive diagnostic.
This patch turns off the AST-based interprocedural analysis in the checker so
that no call path is needed and improves with diagnostic text. With these
changes, the checker is ready to be moved into the optin package.
Ultimately the right fix is to rewrite this checker to be path sensitive -- but
there is still value in enabling the checker for intraprocedural analysis only
The interprocedural mode can be re-enabled with an -analyzer-config flag.
Differential Revision: https://reviews.llvm.org/D26768
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
cfe/trunk/test/Analysis/virtualcall.cpp
cfe/trunk/test/Analysis/virtualcall.h
Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=289309&r1=289308&r2=289309&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Fri Dec 9 19:16:09 2016
@@ -42,6 +42,7 @@ def Nullability : Package<"nullability">
def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, InPackage<Alpha>, Hidden;
+def CplusplusOptIn : Package<"cplusplus">, InPackage<OptIn>;
def Valist : Package<"valist">;
def ValistAlpha : Package<"valist">, InPackage<Alpha>, Hidden;
@@ -262,13 +263,13 @@ def CXXSelfAssignmentChecker : Checker<"
} // end: "cplusplus"
-let ParentPackage = CplusplusAlpha in {
+let ParentPackage = CplusplusOptIn in {
def VirtualCallChecker : Checker<"VirtualCall">,
HelpText<"Check virtual function calls during construction or destruction">,
DescFile<"VirtualCallChecker.cpp">;
-} // end: "alpha.cplusplus"
+} // end: "optin.cplusplus"
//===----------------------------------------------------------------------===//
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp?rev=289309&r1=289308&r2=289309&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp Fri Dec 9 19:16:09 2016
@@ -32,6 +32,18 @@ class WalkAST : public StmtVisitor<WalkA
BugReporter &BR;
AnalysisDeclContext *AC;
+ /// The root constructor or destructor whose callees are being analyzed.
+ const CXXMethodDecl *RootMethod = nullptr;
+
+ /// Whether the checker should walk into bodies of called functions.
+ /// Controlled by the "Interprocedural" analyzer-config option.
+ bool IsInterprocedural = false;
+
+ /// Whether the checker should only warn for calls to pure virtual functions
+ /// (which is undefined behavior) or for all virtual functions (which may
+ /// may result in unexpected behavior).
+ bool ReportPureOnly = false;
+
typedef const CallExpr * WorkListUnit;
typedef SmallVector<WorkListUnit, 20> DFSWorkList;
@@ -59,9 +71,16 @@ class WalkAST : public StmtVisitor<WalkA
const CallExpr *visitingCallExpr;
public:
- WalkAST(const CheckerBase *checker, BugReporter &br,
- AnalysisDeclContext *ac)
- : Checker(checker), BR(br), AC(ac), visitingCallExpr(nullptr) {}
+ WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac,
+ const CXXMethodDecl *rootMethod, bool isInterprocedural,
+ bool reportPureOnly)
+ : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod),
+ IsInterprocedural(isInterprocedural), ReportPureOnly(reportPureOnly),
+ visitingCallExpr(nullptr) {
+ // Walking should always start from either a constructor or a destructor.
+ assert(isa<CXXConstructorDecl>(rootMethod) ||
+ isa<CXXDestructorDecl>(rootMethod));
+ }
bool hasWork() const { return !WList.empty(); }
@@ -132,7 +151,8 @@ void WalkAST::VisitChildren(Stmt *S) {
void WalkAST::VisitCallExpr(CallExpr *CE) {
VisitChildren(CE);
- Enqueue(CE);
+ if (IsInterprocedural)
+ Enqueue(CE);
}
void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
@@ -164,51 +184,64 @@ void WalkAST::VisitCXXMemberCallExpr(Cal
!MD->getParent()->hasAttr<FinalAttr>())
ReportVirtualCall(CE, MD->isPure());
- Enqueue(CE);
+ if (IsInterprocedural)
+ Enqueue(CE);
}
void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
+ if (ReportPureOnly && !isPure)
+ return;
+
SmallString<100> buf;
llvm::raw_svector_ostream os(buf);
- os << "Call Path : ";
- // Name of current visiting CallExpr.
- os << *CE->getDirectCallee();
-
- // Name of the CallExpr whose body is current walking.
- if (visitingCallExpr)
- os << " <-- " << *visitingCallExpr->getDirectCallee();
- // Names of FunctionDecls in worklist with state PostVisited.
- for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(),
+ // FIXME: The interprocedural diagnostic experience here is not good.
+ // Ultimately this checker should be re-written to be path sensitive.
+ // For now, only diagnose intraprocedurally, by default.
+ if (IsInterprocedural) {
+ os << "Call Path : ";
+ // Name of current visiting CallExpr.
+ os << *CE->getDirectCallee();
+
+ // Name of the CallExpr whose body is current being walked.
+ if (visitingCallExpr)
+ os << " <-- " << *visitingCallExpr->getDirectCallee();
+ // Names of FunctionDecls in worklist with state PostVisited.
+ for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(),
E = WList.begin(); I != E; --I) {
- const FunctionDecl *FD = (*(I-1))->getDirectCallee();
- assert(FD);
- if (VisitedFunctions[FD] == PostVisited)
- os << " <-- " << *FD;
+ const FunctionDecl *FD = (*(I-1))->getDirectCallee();
+ assert(FD);
+ if (VisitedFunctions[FD] == PostVisited)
+ os << " <-- " << *FD;
+ }
+
+ os << "\n";
}
PathDiagnosticLocation CELoc =
PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
SourceRange R = CE->getCallee()->getSourceRange();
- if (isPure) {
- os << "\n" << "Call pure virtual functions during construction or "
- << "destruction may leads undefined behaviour";
- BR.EmitBasicReport(AC->getDecl(), Checker,
- "Call pure virtual function during construction or "
- "Destruction",
- "Cplusplus", os.str(), CELoc, R);
- return;
- }
- else {
- os << "\n" << "Call virtual functions during construction or "
- << "destruction will never go to a more derived class";
- BR.EmitBasicReport(AC->getDecl(), Checker,
- "Call virtual function during construction or "
- "Destruction",
- "Cplusplus", os.str(), CELoc, R);
- return;
- }
+ os << "Call to ";
+ if (isPure)
+ os << "pure ";
+
+ os << "virtual function during ";
+
+ if (isa<CXXConstructorDecl>(RootMethod))
+ os << "construction ";
+ else
+ os << "destruction ";
+
+ if (isPure)
+ os << "has undefined behavior";
+ else
+ os << "will not dispatch to derived class";
+
+ BR.EmitBasicReport(AC->getDecl(), Checker,
+ "Call to virtual function during construction or "
+ "destruction",
+ "C++ Object Lifecycle", os.str(), CELoc, R);
}
//===----------------------------------------------------------------------===//
@@ -218,14 +251,18 @@ void WalkAST::ReportVirtualCall(const Ca
namespace {
class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > {
public:
+ DefaultBool isInterprocedural;
+ DefaultBool isPureOnly;
+
void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr,
BugReporter &BR) const {
- WalkAST walker(this, BR, mgr.getAnalysisDeclContext(RD));
+ AnalysisDeclContext *ADC = mgr.getAnalysisDeclContext(RD);
// Check the constructors.
for (const auto *I : RD->ctors()) {
if (!I->isCopyOrMoveConstructor())
if (Stmt *Body = I->getBody()) {
+ WalkAST walker(this, BR, ADC, I, isInterprocedural, isPureOnly);
walker.Visit(Body);
walker.Execute();
}
@@ -234,6 +271,7 @@ public:
// Check the destructor.
if (CXXDestructorDecl *DD = RD->getDestructor())
if (Stmt *Body = DD->getBody()) {
+ WalkAST walker(this, BR, ADC, DD, isInterprocedural, isPureOnly);
walker.Visit(Body);
walker.Execute();
}
@@ -242,5 +280,12 @@ public:
}
void ento::registerVirtualCallChecker(CheckerManager &mgr) {
- mgr.registerChecker<VirtualCallChecker>();
+ VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+ checker->isInterprocedural =
+ mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false,
+ checker);
+
+ checker->isPureOnly =
+ mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false,
+ checker);
}
Modified: cfe/trunk/test/Analysis/virtualcall.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/virtualcall.cpp?rev=289309&r1=289308&r2=289309&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/virtualcall.cpp (original)
+++ cfe/trunk/test/Analysis/virtualcall.cpp Fri Dec 9 19:16:09 2016
@@ -1,36 +1,79 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
+
+/* When INTERPROCEDURAL is set, we expect diagnostics in all functions reachable
+ from a constructor or destructor. If it is not set, we expect diagnostics
+ only in the constructor or destructor.
+
+ When PUREONLY is set, we expect diagnostics only for calls to pure virtual
+ functions not to non-pure virtual functions.
+*/
class A {
public:
A();
+ A(int i);
+
~A() {};
- virtual int foo() = 0;
+ virtual int foo() = 0; // from Sema: expected-note {{'foo' declared here}}
virtual void bar() = 0;
void f() {
- foo(); // expected-warning{{Call pure virtual functions during construction or destruction may leads undefined behaviour}}
+ foo();
+#if INTERPROCEDURAL
+ // expected-warning-re at -2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction has undefined behavior}}
+#endif
}
};
class B : public A {
public:
B() {
- foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+ // expected-warning-re at -3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+ // expected-warning-re at -5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
+
}
~B();
virtual int foo();
- virtual void bar() { foo(); } // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ virtual void bar() { foo(); }
+#if INTERPROCEDURAL
+ // expected-warning-re at -2 {{{{^}}Call Path : foo <-- barCall to virtual function during destruction will not dispatch to derived class}}
+#endif
};
A::A() {
f();
}
+A::A(int i) {
+ foo(); // From Sema: expected-warning {{call to pure virtual member function 'foo' has undefined behavior}}
+#if INTERPROCEDURAL
+ // expected-warning-re at -2 {{{{^}}Call Path : fooCall to pure virtual function during construction has undefined behavior}}
+#else
+ // expected-warning-re at -4 {{{{^}}Call to pure virtual function during construction has undefined behavior}}
+#endif
+}
+
B::~B() {
this->B::foo(); // no-warning
this->B::bar();
- this->foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ this->foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+ // expected-warning-re at -3 {{{{^}}Call Path : fooCall to virtual function during destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re at -5 {{{{^}}Call to virtual function during destruction will not dispatch to derived class}}
+#endif
+#endif
+
}
class C : public B {
@@ -43,7 +86,14 @@ public:
};
C::C() {
- f(foo()); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ f(foo());
+#if !PUREONLY
+#if INTERPROCEDURAL
+ // expected-warning-re at -3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+ // expected-warning-re at -5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
}
class D : public B {
Modified: cfe/trunk/test/Analysis/virtualcall.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/virtualcall.h?rev=289309&r1=289308&r2=289309&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/virtualcall.h (original)
+++ cfe/trunk/test/Analysis/virtualcall.h Fri Dec 9 19:16:09 2016
@@ -18,7 +18,15 @@ namespace header {
class A {
public:
A() {
- foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+ // expected-warning-re at -3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+ // expected-warning-re at -5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
+
}
virtual int foo();
More information about the cfe-commits
mailing list