dcoughlin created this revision.
dcoughlin added reviewers: zaks.anna, NoQ.
dcoughlin added subscribers: cfe-commits, alexfh.
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 evaluated to move out of alpha.
Ultimately the right fix is to rewrite this checker to be path sensitive -- but
there is still value in enabling the checker by default for intraprocedural
analysis only. The interprocedural mode can be re-enabled with an
-analyzer-config flag.
https://reviews.llvm.org/D26768
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
test/Analysis/dtor.cpp
test/Analysis/virtualcall.cpp
test/Analysis/virtualcall.h
Index: test/Analysis/virtualcall.h
===================================================================
--- test/Analysis/virtualcall.h
+++ test/Analysis/virtualcall.h
@@ -18,7 +18,12 @@
class A {
public:
A() {
- foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ foo();
+#if INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during construction or destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction or destruction will not dispatch to derived class}}
+#endif
}
virtual int foo();
Index: test/Analysis/virtualcall.cpp
===================================================================
--- test/Analysis/virtualcall.cpp
+++ test/Analysis/virtualcall.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=cplusplus.VirtualCall -analyzer-store region -analyzer-config cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify -std=c++11 %s
class A {
public:
@@ -8,19 +9,30 @@
virtual int foo() = 0;
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@-2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction or destruction 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 INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during construction or destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction or destruction will not dispatch to derived class}}
+#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@-2 {{{{^}}Call Path : foo <-- barCall to virtual function during construction or destruction will not dispatch to derived class}}
+#endif
};
A::A() {
@@ -30,7 +42,13 @@
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 INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during construction or destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction or destruction will not dispatch to derived class}}
+#endif
+
}
class C : public B {
@@ -43,7 +61,12 @@
};
C::C() {
- f(foo()); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+ f(foo());
+#if INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during construction or destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction or destruction will not dispatch to derived class}}
+#endif
}
class D : public B {
Index: test/Analysis/dtor.cpp
===================================================================
--- test/Analysis/dtor.cpp
+++ test/Analysis/dtor.cpp
@@ -184,25 +184,25 @@
virtual int get() { return 1; }
~A() {
- *out1 = get();
+ *out1 = get(); // expected-warning {{Call to virtual function during construction or destruction will not dispatch to derived class}}
}
};
class B : public A {
public:
virtual int get() { return 2; }
~B() {
- *out2 = get();
+ *out2 = get(); // expected-warning {{Call to virtual function during construction or destruction will not dispatch to derived class}}
}
};
class C : public B {
public:
virtual int get() { return 3; }
~C() {
- *out3 = get();
+ *out3 = get(); // expected-warning {{Call to virtual function during construction or destruction will not dispatch to derived class}}
}
};
Index: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -32,6 +32,10 @@
BugReporter &BR;
AnalysisDeclContext *AC;
+ /// Whether the checker should walk into bodies of called functions.
+ /// Controlled by the "Interprocedural" analyzer-config option.
+ bool IsInterprocedural = false;
+
typedef const CallExpr * WorkListUnit;
typedef SmallVector<WorkListUnit, 20> DFSWorkList;
@@ -60,8 +64,9 @@
public:
WalkAST(const CheckerBase *checker, BugReporter &br,
- AnalysisDeclContext *ac)
- : Checker(checker), BR(br), AC(ac), visitingCallExpr(nullptr) {}
+ AnalysisDeclContext *ac, bool isInterprocedural)
+ : Checker(checker), BR(br), AC(ac), visitingCallExpr(nullptr),
+ IsInterprocedural(isInterprocedural) { }
bool hasWork() const { return !WList.empty(); }
@@ -132,7 +137,8 @@
void WalkAST::VisitCallExpr(CallExpr *CE) {
VisitChildren(CE);
- Enqueue(CE);
+ if (IsInterprocedural)
+ Enqueue(CE);
}
void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
@@ -164,51 +170,56 @@
!MD->getParent()->hasAttr<FinalAttr>())
ReportVirtualCall(CE, MD->isPure());
- Enqueue(CE);
+ if (IsInterprocedural)
+ Enqueue(CE);
}
void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
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 construction or 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,9 +229,11 @@
namespace {
class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > {
public:
+ DefaultBool isInterprocedural;
+
void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr,
BugReporter &BR) const {
- WalkAST walker(this, BR, mgr.getAnalysisDeclContext(RD));
+ WalkAST walker(this, BR, mgr.getAnalysisDeclContext(RD), isInterprocedural);
// Check the constructors.
for (const auto *I : RD->ctors()) {
@@ -242,5 +255,8 @@
}
void ento::registerVirtualCallChecker(CheckerManager &mgr) {
- mgr.registerChecker<VirtualCallChecker>();
+ VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+ checker->isInterprocedural =
+ mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false,
+ checker);
}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -260,16 +260,11 @@
HelpText<"Checks C++ copy and move assignment operators for self assignment">,
DescFile<"CXXSelfAssignmentChecker.cpp">;
-} // end: "cplusplus"
-
-let ParentPackage = CplusplusAlpha in {
-
def VirtualCallChecker : Checker<"VirtualCall">,
- HelpText<"Check virtual function calls during construction or destruction">,
+ HelpText<"Check for virtual function calls during construction or destruction">,
DescFile<"VirtualCallChecker.cpp">;
-} // end: "alpha.cplusplus"
-
+} // end: "cplusplus"
//===----------------------------------------------------------------------===//
// Valist checkers.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
