[analyzer] Re-implemente current virtual calls checker in a path-sensitive way

[wangxindsb]

Summary

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.

[xazax.hun]

I do not see any test cases for this patch. Could you add them as well?

Are you sure that this representation is ok?
How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

VirtualCallChecker.cpp

44 ↗

(On Diff #102812)

The name of this flag is not descriptive enough. Please choose a name that refers to what are you using this value for.

79 ↗

(On Diff #102812)

Variable names should start with uppercase letter.

83 ↗

(On Diff #102812)

Are you sure that the LCtx->getDecl can not return null?

114 ↗

(On Diff #102812)

Do you need this cast here?

119 ↗

(On Diff #102812)

Querying the state is not free, I think you should also query something from the state once you are sure that you will need that.

158 ↗

(On Diff #102812)

I don't think this is the right approach. Once you increased the ObjectFlag on a path, you will never report anything on that path anymore. I think it might be better to have a map from Symbols (representing this) to ctor/dtors or some other approach.

260 ↗

(On Diff #102812)

The formatting seems to be off here I recommend using clang format.

[wangxindsb]

Add test case for the patch

[wangxindsb]

I do not see any test cases for this patch. Could you add them as well?

I add the test case just now.

How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

I use the checker to check the code above, the checker works as expect and doesn't throw the warning.

[xazax.hun]

Note that when you update the differential revision you need to upload the whole diff. Your diff now only contains the tests but not the code.

In D34275#785189, @wangxindsb wrote:

How do you handle the following case?

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
  }
}
int main() {
  A a;
}

I use the checker to check the code above, the checker works as expect and doesn't throw the warning.

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

virtualcall.cpp

1 ↗

(On Diff #103180)

Please add the appropriate run lines so you can run the tests using make check.

[wangxindsb]

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

Yes, the checker warn on the second call.

[wangxindsb]

Add test case for the virtual call checker.

[wangxindsb]

Add run line in the test case.

[wangxindsb]

VirtualCallChecker.cpp

260 ↗

(On Diff #102812)

Sorry, can you explain what this means?

[wangxindsb]

Correct some error in VirtualCallChecker.cpp.
Complete the test case.

[xazax.hun]

In D34275#785294, @wangxindsb wrote:

What about:

struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    foo(); // should warn here
  }
  virtual foo();
}
int main() {
  A a;
}

Does the checker warn on the second call?

Yes, the checker warn on the second call.

Oh, I think I see how it works now. How about:

struct A;
struct X {
   void callFooOfA(A*);
};
struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    x.callFooOfA(this)
  }
  virtual foo();
};
void X::callFooOfA(A* a) {
   a->foo(); // Would be good to warn here. 
}
int main() {
  A a;
}

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

1

Please add the license back.

[wangxindsb]

> Oh, I think I see how it works now. How about:

struct A;
struct X {
   void callFooOfA(A*);
};
struct A {
  A() {
    X x;
    x.virtualMethod(); // this virtual call is ok
    x.callFooOfA(this)
  }
  virtual foo();
};
void X::callFooOfA(A* a) {
   a->foo(); // Would be good to warn here. 
}
int main() {
  A a;
}

Yes, you are right, the checker doesn't warn on the a->foo();. I will fix this error soon.

Thanks,
Xin

[wangxindsb]

Add license to VirtualCallChecker.cpp

[wangxindsb]

-Fix the bug of the virtual call during a function call of the object.
-Add flag for the PUREONLY.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

105

Could you find more descriptive names for these BugTypes?

150–152

Comments should be full sentences, this means they should be terminated with a period.

151

Variables should start with capital letters.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

106

I'd rather have one bug type, describing a call to a virtual function from ctor/dtor. The actual error message can clarify whether this is a call from ctor or from dtor and whether it is pure virtual. You do not need to reflect every detail in the bug type.

156

The names of the arguments should start with an uppercase letter.

175

Do you need these traits above after having the maps bellow?

199

The formatting here seems to be off, could you run clang-format on the code? This is a tool that can format the code to comply with the LLVM formatting style guide.

207–212

I think you do not need the or_null suffix here since the argument of this cast will never be null.

235

If you do not use the result of dyn_cast, you could use isa instead. Even better, you could reuse CC or DC in this check.

236

Shouldn't this be const?

250

In LLVM we do not write the braces for conditionals that guarding a single statement.

265

In case of calling a pure virtual function maybe it would be better to stop the analysis, i.e.: creating a fatal error node.
Also, these functions might fail to give you back an ExplodedNode, so I think you should have a check here for that case.

277

Maybe instead of callExpr, you should get CXXInstanceCall, which has a getCXXThisVal method.

285

Can getParent ever return null? Maybe you do not need the or_null suffix in the cast.
Moreover are you sure that this method works? In case the function was a method, the parent should be the CXXRecordDecl, not a CXXMethodDecl.

[wangxindsb]

• Rename the BugType.
• Correct the code style to be clang format.
• Rename the arguments to start with uppercase letter.
• Add the support to check for the constructor called by the New expr.

You don't need to put effort to review the code, because I will make some changes to the bug report system and the GDM of the checker which has not been done now.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

277

I tried to use the CXXInstanceCall, but I can't use it to get the expr which call the function.

[wangxindsb]

• Change the two bugtype to one.
• Use the generateErrorNode() for the pure virtual call.
• Change the test case for the new expression.

[wangxindsb]

• Change the two bugtype to one.
• Use the generateErrorNode() for the pure virtual call.
• Change the test case for the new expression.

[wangxindsb]

• Use the map of object to ctor/dtor to check the virtual call.
• Use CXXInstanceCall and getCXXThisVal method to get the 'this' instead of getThisSVal().
• Correct some format errors.

[wangxindsb]

Look forward to your review.

[xazax.hun]

You are making a pretty good progress!
I think right now there is some code duplication that could be reduced, but otherwise, the checker starts to look really good.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

134

This could be maybe a free static function instead of a method?

138

Maybe instead of Region the name could be more specific? I.e. the ThisRegion? Or ObjectRegion?

195

In the warning messages we usually enclose the source code snippets (e.g.: declaration names) in single quotes.

208

getting the SValBuilder is cheap, maybe you could hoist it from the if statements.

227

The code of this function is almost completely identical with the checkBegin. I was thinking about abstracting this into a separate function with a bool argument to determine whether we would like to add something to the GDM or remove it.

255–256

Maybe you could cast this to CXXMemberCall instead. If that cast succeed, than you do not need to check the Call.getDecl() and also do not need to check for ctor/dtor calls.

258

Maybe it would be cleaner to early return if the call is not virtual and remove it from these two if statements.

272

Maybe you could extract the error reporting into a separate function with two arguments: the error message, and whether it should generate an error node.

[wangxindsb]

• Change IsVirtualCall(const CallExpr *CE) to be a free static function.
• Rename some variables.
• Improve the BugReporterVisitor, enclose the declaration names in single quotes.
• Hoist getSValBuilder() from the if statements.
• Fix some code duplications.
• Use the CXXMemberCall instead CXXInstanceCall in the CheckPreCall.
• Remove IsVirtualCall(CE) from if statements.
• Fix the error of the visitnode() method which may throw a wrong call graph for the code blow.

class Y {
public:
  virtual void foobar();
  Y() {
    F f1;
    foobar();
  }
};

Previous visitnode() will issue the virtual function called from the F(); Current visitnode() fix this bug.

[xazax.hun]

Thank you! I think we can start to run this check on real world code bases and evaluate the results.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

157

Function names should start with a lower case letter. These two could be private. I'd prefer another name for this function like registerCtorDtorCallInState.

158

Use StringRef instead of const char *. It is more idiomatic in LLVM code.

263

I'd omit the Msg local variable. After that, we do not need to use braces for single statement blocks.

280

Do not use braces for single statement blocks.

281

Braces for single statement blocks.

284

Braces for single statement blocks.

[wangxindsb]

• Change function name to start with a lower case letter.
• Use StringRef instead of `const char * ' for ReportBug() const.
• Correct the braces for single statement blocks.

[xazax.hun]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

236

I was wondering if there is another way to represent the state.
We could have a two element (bool based) enum class like:

enum class ObjectState : bool {
  CtorCalled,
  DtorCalled
};

Se we could have only one map in the GDM. Either the destructor is called for an object or the constructor. Or in case none of them is called on a path, the state is empty. What do you think?

250

You could move the declarations of PSM and SVB after the next early return.

255–256

These two checks could be moved before you query the CXXThisVal.

271

You could eliminate this local variable.

279

This return is redundant.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

236

Yes, it's better than the previous two maps.

[wangxindsb]

• Change two maps to one map.
• Move the declarations of PSM and SVB after the next early return.
• Delete the variable std::string DeclName.
• Delete last return in reportbug().

[wangxindsb]

• Change the bugtype, just like the older checker.

[wangxindsb]

Fix the Assertion Failed when run the checker to check the building of LibreOffice.

[xazax.hun]

@NoQ , @dcoughlin could either of you review this patch as well? The end of GSoC is closing and it would be awesome to be able to commit this before it ends. :)

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

30

Missing namespace closing comment.

[wangxindsb]

+enum class ObjectState : bool { CtorCalled, DtorCalled };
+} // end namespace

Add namespace closing comment.

[NoQ]

First of all, thanks everybody for working on this. I'd see what i can do.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

29

Please remind me what was wrong with ascending over StackFrameContexts to see if we're inside a constructor or destructor call? Why did we need a state trait here? Was it too hard to obtain this-values? (Not sure if there's time to fix this, but it might be worth it to leave a FIXME around).

161

I wish for camelCase.

233

I don't think this is working. CXXThisRegion is never a region of a C++ object; it's the segment of the stack where the pointer is passed, you need to dereference its value to get the actual object region.

Probably tests for the visitor might make things more clear.

264

"Pure function" doesn't mean what we want to say here (https://en.wikipedia.org/wiki/Pure_function), i think we should stick with "pure virtual function" anyways.

test/Analysis/virtualcall.cpp

16

Tests need fixing: expected-warning requires {{ and needs to be on the correct line.

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

29

I have implement this method before, the code is on this link, virtualcallchecker using stackframe. I thought it was harder and slower than the state trait, so I gave up this method. Also, I thought this method maybe hard to add visitor. I will work on this soon.

233

class Y {
public:
  virtual void foobar();
  void fooY() {  
    F f1;
    foobar();
  }
  Y() {
    fooY();
  }
};

I thought this test is for this situation. If the visitor is correct, it will issued "Called from this constructor 'Y'", else it will issued "Called from this constructor 'F'".

[wangxindsb]

• Rename reportbug();
• Change message "Pure function" to "pure virtual function";
• Fixing: expected-warning;

[wangxindsb]

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

111

Add the FIXME

[NoQ]

Tests are still not working - they pass now, but they don't actually test anything. Please make sure that tests actually work. Which means that

1. Tests pass when you run make -j4 check-clang-analysis;
2. Tests start failing when you change your code or expected-warning directives.

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

233

Right, i didn't notice getSVal(); seems correct.

This test doesn't verify anything though, because it has no expected-warnings. Even if it had, it wouldn't verify where the visitor diagnostic appears, because without a more specific -analyzer-output option (eg. =text), the analyzer wouldn't emit visitor notes, so the -verify option would not be able to verify them.

So by visitor tests i mean adding -analyzer-output=text to the run-line and then adding expected-note directives wherever notes are expected.

246–249

Typo: "constructor", "destructor".

Also i guess we need to think about this warning message more carefully anyway. Like, we already have an "entering function..." event diagnostic piece. Because the warning always happens inside the function in question, this event would never be pruned, so it'd always be there. So what do we expect the visitor's diagnostic piece to say?

I suggest "This [constructor|destructor] of an object of type 'Foo' has not returned when the virtual method was called". With a probable future improvement of specifying the name of the object (when eg. it's a variable).

It might even be that later we'd decide that the visitor is not necessary for this checker. I'm not sure, i guess i'd have to see how it works in practice.

Also, right now the visitor's piece is above the "entering function..." event. Not sure if having it below, or even inside the constructor, would be better.

test/Analysis/virtualcall.cpp

3–4

Tests are still not working because your auto-format tool has inserted weird line breaks.

[NoQ]

Most importantly, do you think we can enable the checker by default now? Was this checker evaluated on any large codebase, and if so, how many true/false positives did it find, probably compared to the old checker?

[wangxindsb]

• Fix the errors of the tests;
• Add -analyzer-output=text to the run-line;
• Change the message of the visitor's diagnostic piece.

[wangxindsb]

@NoQ

In D34275#847434, @NoQ wrote:

Most importantly, do you think we can enable the checker by default now? Was this checker evaluated on any large codebase, and if so, how many true/false positives did it find, probably compared to the old checker?

I am now runing the checker to check the build of firefox, llvm and libreoffice. There are no alarms on the building of firefox and llvm. When use scan-build to check the building of libreoffice, the scan-build failed (this may because the scan-build. Enable or not enable the virtualcallchecker, the build failed all the same, but when I just build the libreoffice and not run the scan-build, the build worked well) and there are 105 alarms about the virtual function in ctors/dtors, I am evaluating these alarms now.

[NoQ]

Thanks, works now! Apart from the minor comment on the hanging header file in the tests, this looks good and i have no further nits :)

*summons @dcoughlin to have a look at English in the warning messages*

test/Analysis/virtualcall.cpp

15–16

I think it might be worth it to highlight that the function is pure virtual even in non-pure-only mode (if you have time for that).

137–141

There used to be a test case in this header; we should restore the test or remove the file if it's no longer relevant.

[wangxindsb]

• Highlight pure virtual even in non-pure-only mode;
• Add change to the header.

[wangxindsb]

There are 105 alarms running the checker on the LibreOffice, 92 True positive, 13 not sure.

[NoQ]

All right then, i approve!

There are 105 alarms running the checker on the LibreOffice, 92 True positive, 13 not sure.

That's impressively loud. I guess you can try reporting some of the bugs you've found and seeing their reactions, if you enjoy this sort of stuff (it's always the ultimate test for bug-finding tools, and it'd also hopefully make libreoffice better).

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

 //=======- VirtualCallChecker.cpp --------------------------------*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines a checker that checks virtual function calls during
 //  construction or destruction of C++ objects.
 //
 //===----------------------------------------------------------------------===//
 
 #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 VirtualCallChecker: public Checker<check::PreCall, check::PostCall> {
+  mutable std::unique_ptr<BugType> BT_CT;
+  mutable std::unique_ptr<BugType> BT_DT;
 
-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. */
-  };
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+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 TrackedCtorDtorFlag;
+    bool Found;
+  public:
+    VirtualBugVisitor(const unsigned TrackedCtorDtorFlag) : 
+      TrackedCtorDtorFlag(TrackedCtorDtorFlag), Found(false) {}
+    void Profile(llvm::FoldingSetNodeID &ID) const override{
+      static int x = 0;
+      ID.AddPointer(&x);
+      ID.AddPointer(&TrackedCtorDtorFlag);
+    }
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                   const ExplodedNode *PrevN,
+                                                   BugReporterContext &BRC,
+                                                   BugReport &BR) override;
+  };
+}
 
-  /// A DenseMap that records visited states of FunctionDecls.
-  llvm::DenseMap<const FunctionDecl *, Kind> VisitedFunctions;
+//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_or_null<CXXConstructorDecl>(LCtx->getDecl());
+  const CXXDestructorDecl *DD =
+        dyn_cast_or_null<CXXDestructorDecl>(LCtx->getDecl());  
+  if((!CD && !DD) || (Ctorflag!=TrackedCtorDtorFlag && 
+      Dtorflag!=TrackedCtorDtorFlag)) 
+    return nullptr;
+  
+  const Stmt *S = PathDiagnosticLocation::getStmt(N); 
+  if (!S)
+    return nullptr;
+  Found = true;
+
+  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;
+  }
+
+  // Generate the extra diagnostic.
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                             N->getLocationContext());
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
+}
 
-  /// 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;
+void VirtualCallChecker::checkPreCall(const CallEvent &Call, 
+                                      CheckerContext &C) const {
 
-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));
-  }
-
-  bool hasWork() const { return !WList.empty(); }
-
-  /// 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);
+  const Decl *D = Call.getDecl();
+  if (!D)
+
+  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);
+
+  // 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);
   }
 
-  /// 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());
+  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 (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);
+        }
+      }
+    }
+  } 
 
-        // Mark the function as being PostVisited to indicate we have
-        // scanned the body.
-        VisitedFunctions[FD] = PostVisited;
-        continue;
-      }
+  // First Check if a virtual method is called, then check the 
+  // GDM of constructor and destructor. 
+  if (isVirtualCall(CE) && State->get<ConstructorFlag>() > 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);
+    const unsigned Ctorflag = State->get<ConstructorFlag>(); 
+    Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(Ctorflag));
+    C.emitReport(std::move(Reporter));
+    return;
 
-      // Otherwise, the callee is PostVisited.
-      // Remove it from the worklist.
-      assert(VisitedFunctions[FD] == PostVisited);
-      WList.pop_back();
-    }
-  }
+  if (isVirtualCall(CE) && State->get<DestructorFlag>() > 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);
+    const unsigned Dtorflag = State->get<DestructorFlag>(); 
+    Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(Dtorflag));
+    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);
+// The PostCall callback, when leave a constructor or a destructor, 
+// decrease the corresponding integer
+void VirtualCallChecker::checkPostCall(const CallEvent &Call, 
+                                       CheckerContext &C) const {
 
-};
-} // end anonymous namespace
+  const Decl *D = 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);
+
+  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);
+  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);
+  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);
-  bool callIsNonVirtual = false;
+// 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;
+      CallIsNonVirtual = true;
 
-      // If the most derived class is marked final, we know that now subclass
-      // can override this member.
-      if (base->getBestDynamicClassType()->hasAttr<FinalAttr>())
-        callIsNonVirtual = true;
+    if (Expr *Base = CME->getBase()->IgnoreImpCasts()) {
+/*      if (!isa<CXXThisExpr>(Base))
+        return false;*/
+
+      // 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>() &&
+  if (MD && MD->isVirtual() && !CallIsNonVirtual && !MD->hasAttr<FinalAttr>() &&
       !MD->getParent()->hasAttr<FinalAttr>())
-    ReportVirtualCall(CE, MD->isPure());
-
-  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);
-
-  // 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";
-  }
-
-  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();
-        }
-    }
+    return true;
+  return false;
 
-    // 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();
-      }
+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;
   }
-};
 }
 
 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>();
 }

test/Analysis/virtualcall.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify -std=c++11 %s
-// RUN: %clang_analyze_cc1 -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.
-*/
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
 
 class A {
 public:
   A();
   A(int i);
 
   ~A() {};
   
-  virtual int foo() = 0; // from Sema: expected-note {{'foo' declared here}}
-  virtual void bar() = 0;
+  virtual int foo(); // from Sema: expected-note {{'foo' declared here}}
+  virtual void bar();
   void f() {
     foo();
-#if INTERPROCEDURAL
-        // expected-warning-re@-2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction has undefined behavior}}
-#endif
+        // expected-warning:Call to virtual function during construction
   }
 };
 
 class B : public A {
 public:
   B() {
     foo();
-#if !PUREONLY
-#if INTERPROCEDURAL
-        // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
-#else
-        // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
-#endif
-#endif
-
+        // expected-warning:Call to virtual function during construction
   }
   ~B();
   
   virtual int foo();
   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
+      // expected-warning:Call to virtual function during destruction
 };
 
 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
+      // expected-warning:Call to virtual function during construction
 }
 
 B::~B() {
   this->B::foo(); // no-warning
   this->B::bar();
   this->foo();
-#if !PUREONLY
-#if INTERPROCEDURAL
-      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during destruction will not dispatch to derived class}}
-#else
-      // expected-warning-re@-5 {{{{^}}Call to virtual function during destruction will not dispatch to derived class}}
-#endif
-#endif
-
+      // expected-warning:Call to virtual function during destruction
 }
 
 class C : public B {
 public:
   C();
   ~C();
   
   virtual int foo();
   void f(int i);
 };
 
 C::C() {
   f(foo());
-#if !PUREONLY
-#if INTERPROCEDURAL
-      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
-#else
-      // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
-#endif
-#endif
+      // expected-warning:Call to virtual function during construction
 }
 
 class D : public B {
 public:
   D() {
     foo(); // no-warning
   }
   ~D() { bar(); }
   int foo() final;
   void bar() final { foo(); } // no-warning
 };
 
 class E final : public B {
 public:
   E() {
     foo(); // no-warning
   }
   ~E() { bar(); }
   int foo() override;
 };
 
-// Regression test: don't crash when there's no direct callee.
 class F {
 public:
   F() {
     void (F::* ptr)() = &F::foo;
     (this->*ptr)();
   }
   void foo();
 };
 
-int main() {
-  A *a;
-  B *b;
-  C *c;
-  D *d;
-  E *e;
-  F *f;
+class G {
+public:
+  virtual void bar();
+  void foo() {
+    bar();
+      // no warning
+  }
+};
+
+class H{
+public:
+  H() : initState(0) { init(); }
+  int initState;
+  virtual void f() const;
+  void init() {
+    if (initState)
+      f();
+      // no warning
   }
 
-#include "virtualcall.h"
-
-#define AS_SYSTEM
-#include "virtualcall.h"
-#undef AS_SYSTEM
+  H(int i) {
+    G g;
+    g.foo();
+    g.bar();
+      // no warning
+    f();
+      // expected-warning:Call to virtual function during construction
+    H& h = *this;
+    h.f();
+      // expected-warning:Call to virtual function during construction
+  }
+};
+
+class X {
+public:
+  X() {
+    g();
+      // expected-warning:Call to virtual function during construction
+  }
+  X(int i) {
+    if (i > 0) {
+      X x(i-1);
+      x.g();
+      // no warning
+    }
+    g();
+      // expected-warning:Call to virtual function during construction
+  }
+  virtual void g();
+};
+
+int main() {
+  A a;
+  A a1(1);
+  B b;
+  C c;
+  D d;
+  E e;
+  F f;
+  G g;
+  H h;
+  H h1(1);
+  X x;
+  X x1(1);
+}