dcoughlin updated this revision to Diff 79981.
dcoughlin added a comment.
- Update the diagnostic to be explicit about whether the issue occurs during
construction or destruction
- Add test for pure, intraprocedural case (Sema also catches this).
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 will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
}
virtual int foo();
Index: test/Analysis/virtualcall.cpp
===================================================================
--- test/Analysis/virtualcall.cpp
+++ test/Analysis/virtualcall.cpp
@@ -1,36 +1,65 @@
-// 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:
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@-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 INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction 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 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@-2 {{{{^}}Call Path : fooCall to pure virtual function during construction has undefined behavior}}
+#else
+ // expected-warning-re@-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 INTERPROCEDURAL
+ // expected-warning-re@-2 {{{{^}}Call Path : fooCall to virtual function during destruction will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during destruction will not dispatch to derived class}}
+#endif
+
}
class C : public B {
@@ -43,7 +72,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 will not dispatch to derived class}}
+#else
+ // expected-warning-re@-4 {{{{^}}Call to virtual function during construction 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 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 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 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,13 @@
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;
+
typedef const CallExpr * WorkListUnit;
typedef SmallVector<WorkListUnit, 20> DFSWorkList;
@@ -59,9 +66,14 @@
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)
+ : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod),
+ IsInterprocedural(isInterprocedural), 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 +144,8 @@
void WalkAST::VisitCallExpr(CallExpr *CE) {
VisitChildren(CE);
- Enqueue(CE);
+ if (IsInterprocedural)
+ Enqueue(CE);
}
void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
@@ -164,51 +177,61 @@
!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 ";
+
+ 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,29 +241,36 @@
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));
+ 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);
walker.Visit(Body);
walker.Execute();
}
}
// Check the destructor.
if (CXXDestructorDecl *DD = RD->getDestructor())
if (Stmt *Body = DD->getBody()) {
+ WalkAST walker(this, BR, ADC, DD, isInterprocedural);
walker.Visit(Body);
walker.Execute();
}
}
};
}
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
