wangxindsb created this revision.
This implement a path-sensitive checker that warns if virtual calls are made
from constructors and destructors.
The checker use the GDM (generic data map) to store three integers in the
program state for constructors, destructors and objects. Once enter one of
these is entered, increase the corresponding integer, once leave, decrease the
corresponding integer. In a PreCall callback, the checker first check if a
virtual method is called and the GDM meets the conditions, if yes, a warning
will be issued.
The checker also include a bug reporter visitor to add an extra note when the
last constructor/destructor was called before the call to the virtual function.
https://reviews.llvm.org/D34275
Files:
VirtualCallChecker.cpp
Index: VirtualCallChecker.cpp
===================================================================
--- VirtualCallChecker.cpp
+++ VirtualCallChecker.cpp
@@ -14,279 +14,269 @@
#include "ClangSACheckers.h"
#include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtVisitor.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
-#include "llvm/ADT/SmallString.h"
-#include "llvm/Support/SaveAndRestore.h"
-#include "llvm/Support/raw_ostream.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h"
using namespace clang;
using namespace ento;
namespace {
-class WalkAST : public StmtVisitor<WalkAST> {
- const CheckerBase *Checker;
- 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;
-
- /// A vector representing the worklist which has a chain of CallExprs.
- DFSWorkList WList;
-
- // PreVisited : A CallExpr to this FunctionDecl is in the worklist, but the
- // body has not been visited yet.
- // PostVisited : A CallExpr to this FunctionDecl is in the worklist, and the
- // body has been visited.
- enum Kind { NotVisited,
- PreVisited, /**< A CallExpr to this FunctionDecl is in the
- worklist, but the body has not yet been
- visited. */
- PostVisited /**< A CallExpr to this FunctionDecl is in the
- worklist, and the body has been visited. */
- };
+class VirtualCallChecker: public Checker<check::PreCall, check::PostCall> {
+ mutable std::unique_ptr<BugType> BT_CT;
+ mutable std::unique_ptr<BugType> BT_DT;
- /// A DenseMap that records visited states of FunctionDecls.
- llvm::DenseMap<const FunctionDecl *, Kind> VisitedFunctions;
+public:
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
- /// The CallExpr whose body is currently being visited. This is used for
- /// generating bug reports. This is null while visiting the body of a
- /// constructor or destructor.
- const CallExpr *visitingCallExpr;
+private:
+ bool isVirtualCall(const CallExpr *CE) const;
+ Optional<SVal> getThisSVal(const StackFrameContext *SFC,
+ const ProgramStateRef State) const;
+ class VirtualBugVisitor : public BugReporterVisitorImpl<VirtualBugVisitor> {
+ private:
+ const unsigned Flag;
+ bool Found;
+
+ public:
+ VirtualBugVisitor(const unsigned Flag) : Flag(Flag), Found(false) {}
+ void Profile(llvm::FoldingSetNodeID &ID) const override{
+ static int x = 0;
+ ID.AddPointer(&x);
+ ID.AddPointer(&Flag);
+ }
+ std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+ };
+};
+} // end anonymous namespace
-public:
- 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));
+// GDM (generic data map) to store two integers in the program state.
+// One integer for constructors, one integer for destructors.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ConstructorFlag, unsigned)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(DestructorFlag, unsigned)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectFlag, unsigned)
+
+std::shared_ptr<PathDiagnosticPiece>
+VirtualCallChecker::VirtualBugVisitor::VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ // We need the last ctor/dtor which call the virtual function
+ // The visitor walks the ExplodedGraph backwards.
+ if (Found)
+ return nullptr;
+
+ ProgramStateRef state = N->getState();
+ const unsigned ctorflag = state->get<ConstructorFlag>();
+ const unsigned dtorflag = state->get<DestructorFlag>();
+ const LocationContext* LCtx = N->getLocationContext();
+ const CXXConstructorDecl *CD =
+ dyn_cast<CXXConstructorDecl>(LCtx->getDecl());
+ const CXXDestructorDecl *DD =
+ dyn_cast<CXXDestructorDecl>(LCtx->getDecl());
+ if((!CD && !DD) || (ctorflag!=Flag && dtorflag!=Flag)) return nullptr;
+
+ // Retrieve the associated statement.
+ const Stmt *S = PathDiagnosticLocation::getStmt(N);
+ if (!S)
+ return nullptr;
+ Found = true;
+
+ // Get the name of the last ctor/dtor which called the virtual function
+ std::string DeclName;
+ std::string InfoText;
+ if(CD) {
+ DeclName = CD->getNameAsString();
+ InfoText = "Called from this constrctor " + DeclName;
+ }
+ else {
+ DeclName = DD->getNameAsString();
+ InfoText = "called from this destructor " + DeclName;
}
- bool hasWork() const { return !WList.empty(); }
+ // Generate the extra diagnostic.
+ PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ N->getLocationContext());
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
+}
+
+void VirtualCallChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ const Decl *D = dyn_cast_or_null<Decl>(Call.getDecl());
+ if (!D)
+ return;
- /// This method adds a CallExpr to the worklist and marks the callee as
- /// being PreVisited.
- void Enqueue(WorkListUnit WLUnit) {
- const FunctionDecl *FD = WLUnit->getDirectCallee();
- if (!FD || !FD->getBody())
- return;
- Kind &K = VisitedFunctions[FD];
- if (K != NotVisited)
- return;
- K = PreVisited;
- WList.push_back(WLUnit);
+ ProgramStateRef state = C.getState();
+ const unsigned ctorflag = state->get<ConstructorFlag>();
+ const unsigned dtorflag = state->get<DestructorFlag>();
+ const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+ const LocationContext *LCtx = C.getLocationContext();
+ const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
+ Optional<SVal> ThisSVal = getThisSVal(SFC,state);
+
+ // Enter a constructor, increase the corresponding integer.
+ if (dyn_cast<CXXConstructorDecl>(D)) {
+ unsigned constructorflag = state->get<ConstructorFlag>();
+ state = state->set<ConstructorFlag>(++constructorflag);
+ C.addTransition(state);
+ return;
}
- /// This method returns an item from the worklist without removing it.
- WorkListUnit Dequeue() {
- assert(!WList.empty());
- return WList.back();
+ // Enter a Destructor, increase the corresponding integer.
+ if (dyn_cast<CXXDestructorDecl>(D)) {
+ unsigned destructorflag = state->get<DestructorFlag>();
+ state = state->set<DestructorFlag>(++destructorflag);
+ C.addTransition(state);
+ return;
}
- void Execute() {
- while (hasWork()) {
- WorkListUnit WLUnit = Dequeue();
- const FunctionDecl *FD = WLUnit->getDirectCallee();
- assert(FD && FD->getBody());
-
- if (VisitedFunctions[FD] == PreVisited) {
- // If the callee is PreVisited, walk its body.
- // Visit the body.
- SaveAndRestore<const CallExpr *> SaveCall(visitingCallExpr, WLUnit);
- Visit(FD->getBody());
-
- // Mark the function as being PostVisited to indicate we have
- // scanned the body.
- VisitedFunctions[FD] = PostVisited;
- continue;
+ // Check if a object is entered, if yes, increase the corresponding integer.
+ if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
+ if (Expr *base = CME->getBase()->IgnoreImpCasts()) {
+ if (!isa<CXXThisExpr>(base)) {
+ SVal CEV = state->getSVal(base, LCtx);
+ if(CEV != ThisSVal){
+ unsigned objectflag = state->get<ObjectFlag>();
+ state = state->set<ObjectFlag>(++objectflag);
+ C.addTransition(state);
+ }
}
+ }
+ }
- // Otherwise, the callee is PostVisited.
- // Remove it from the worklist.
- assert(VisitedFunctions[FD] == PostVisited);
- WList.pop_back();
+ // First Check if a virtual method is called, then check the
+ // GDM of constructor and destructor.
+ if (isVirtualCall(CE) && ctorflag > 0 && state->get<ObjectFlag>() == 0) {
+ if (!BT_CT) {
+ BT_CT.reset(new BugType(this,
+ "Call to virtual function during construction", "not pure"));
}
+ ExplodedNode *N = C.generateNonFatalErrorNode();
+ auto Reporter = llvm::make_unique<BugReport>(*BT_CT, BT_CT->getName(), N);
+ Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(ctorflag));
+ C.emitReport(std::move(Reporter));
+ return;
}
- // Stmt visitor methods.
- void VisitCallExpr(CallExpr *CE);
- void VisitCXXMemberCallExpr(CallExpr *CE);
- void VisitStmt(Stmt *S) { VisitChildren(S); }
- void VisitChildren(Stmt *S);
-
- void ReportVirtualCall(const CallExpr *CE, bool isPure);
+ if (isVirtualCall(CE) && dtorflag > 0 && state->get<ObjectFlag>() == 0) {
+ if (!BT_DT) {
+ BT_DT.reset(new BugType(this,
+ "Call to virtual function during destruction", "not pure"));
+ }
+ ExplodedNode *N = C.generateNonFatalErrorNode();
+ auto Reporter = llvm::make_unique<BugReport>(*BT_DT, BT_DT->getName(), N);
+ Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(dtorflag));
+ C.emitReport(std::move(Reporter));
+ return;
+ }
+}
-};
-} // end anonymous namespace
+// The PostCall callback, when leave a constructor or a destructor,
+// decrease the corresponding integer.
+void VirtualCallChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ const Decl *D = dyn_cast_or_null<Decl>(Call.getDecl());
+ if (!D)
+ return;
-//===----------------------------------------------------------------------===//
-// AST walking.
-//===----------------------------------------------------------------------===//
+ ProgramStateRef state = C.getState();
+ const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+ const LocationContext *LCtx = C.getLocationContext();
+ const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
+ Optional<SVal> ThisSVal = getThisSVal(SFC,state);
+
+ // Leave a constructor, decrease the corresponding integer.
+ if (dyn_cast<CXXConstructorDecl>(D)) {
+ unsigned constructorflag = state->get<ConstructorFlag>();
+ state = state->set<ConstructorFlag>(--constructorflag);
+ C.addTransition(state);
+ return;
+ }
-void WalkAST::VisitChildren(Stmt *S) {
- for (Stmt *Child : S->children())
- if (Child)
- Visit(Child);
-}
+ // Leave a Destructor, decrease the corresponding integer.
+ if (dyn_cast<CXXDestructorDecl>(D)) {
+ unsigned destructorflag = state->get<DestructorFlag>();
+ state = state->set<DestructorFlag>(--destructorflag);
+ C.addTransition(state);
+ return;
+ }
-void WalkAST::VisitCallExpr(CallExpr *CE) {
- VisitChildren(CE);
- if (IsInterprocedural)
- Enqueue(CE);
+ // Leave the object, decrease the corresponding integer.
+ if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
+ if (Expr *base = CME->getBase()->IgnoreImpCasts()) {
+ if (!isa<CXXThisExpr>(base)) {
+ SVal CEV = state->getSVal(base, LCtx);
+ if(CEV != ThisSVal){
+ unsigned objectflag = state->get<ObjectFlag>();
+ state = state->set<ObjectFlag>(--objectflag);
+ C.addTransition(state);
+ }
+ }
+ }
+ }
}
-void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
- VisitChildren(CE);
+// The function to check if a virtual function is called
+bool VirtualCallChecker::isVirtualCall(const CallExpr *CE) const {
bool callIsNonVirtual = false;
- // Several situations to elide for checking.
- if (MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
+ if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
// If the member access is fully qualified (i.e., X::F), then treat
// this as a non-virtual call and do not warn.
if (CME->getQualifier())
callIsNonVirtual = true;
if (Expr *base = CME->getBase()->IgnoreImpCasts()) {
- // Elide analyzing the call entirely if the base pointer is not 'this'.
- if (!isa<CXXThisExpr>(base))
- return;
-
- // If the most derived class is marked final, we know that now subclass
- // can override this member.
+ // The most derived class is marked final.
if (base->getBestDynamicClassType()->hasAttr<FinalAttr>())
callIsNonVirtual = true;
}
}
- // Get the callee.
const CXXMethodDecl *MD =
dyn_cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
- if (MD && MD->isVirtual() && !callIsNonVirtual && !MD->hasAttr<FinalAttr>() &&
- !MD->getParent()->hasAttr<FinalAttr>())
- ReportVirtualCall(CE, MD->isPure());
-
- if (IsInterprocedural)
- Enqueue(CE);
+ if (MD && MD->isVirtual() && !callIsNonVirtual && !MD->hasAttr<FinalAttr>()
+ && !MD->getParent()->hasAttr<FinalAttr>())
+ return true;
+ return false;
}
-void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
- if (ReportPureOnly && !isPure)
- return;
-
- SmallString<100> buf;
- llvm::raw_svector_ostream os(buf);
-
- // 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;
- }
-
- os << "\n";
+// Get the symbolic value of the "this" object for a method call that created
+// the given stack frame. Returns None if the stack frame does not represent
+// a method call.
+Optional<SVal>
+VirtualCallChecker::getThisSVal(const StackFrameContext *SFC,
+ const ProgramStateRef state) const {
+ if (SFC->inTopFrame()) {
+ const FunctionDecl *FD = SFC->getDecl()->getAsFunction();
+ if (!FD)
+ return None;
+ const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(FD->getParent());
+ if (!MD)
+ return None;
+ Loc ThisLoc = state->getStateManager().getSValBuilder().getCXXThis(MD, SFC);
+ return state->getSVal(ThisLoc);
+ } else {
+ const Stmt *S = SFC->getCallSite();
+ if (!S)
+ return None;
+ if (const CXXMemberCallExpr *MCE = dyn_cast_or_null<CXXMemberCallExpr>(S))
+ return state->getSVal(MCE->getImplicitObjectArgument(), SFC->getParent());
+ else if (const CXXConstructExpr *CCE = dyn_cast_or_null<CXXConstructExpr>(S))
+ return state->getSVal(CCE, SFC->getParent());
+ return None;
}
-
- PathDiagnosticLocation CELoc =
- PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
- SourceRange R = CE->getCallee()->getSourceRange();
-
- 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);
-}
-
-//===----------------------------------------------------------------------===//
-// VirtualCallChecker
-//===----------------------------------------------------------------------===//
-
-namespace {
-class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > {
-public:
- DefaultBool isInterprocedural;
- DefaultBool isPureOnly;
-
- void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr,
- BugReporter &BR) const {
- 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();
- }
- }
-
- // 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();
- }
- }
-};
}
void ento::registerVirtualCallChecker(CheckerManager &mgr) {
- VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
- checker->isInterprocedural =
- mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false,
- checker);
-
- checker->isPureOnly =
- mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false,
- checker);
+ mgr.registerChecker<VirtualCallChecker>();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
