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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:31
+class VirtualCallChecker: public Checker<check::PreCall, check::PostCall> {
+  mutable std::unique_ptr<BugType> BT_CT;
+  mutable std::unique_ptr<BugType> BT_DT;
----------------
Could you find more descriptive names for these BugTypes?


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:32
+  mutable std::unique_ptr<BugType> BT_CT;
+  mutable std::unique_ptr<BugType> BT_DT;
 
----------------
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. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:35
 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);
-  }
-
-  /// This method returns an item from the worklist without removing it.
-  WorkListUnit Dequeue() {
-    assert(!WList.empty());
-    return WList.back();
-  }
-
-  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;
-      }
+  // The flag to determine if pure virtual functions should be issued only
+  DefaultBool isPureOnly;
----------------
Comments should be full sentences, this means they should be terminated with a period. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:36
+  // The flag to determine if pure virtual functions should be issued only
+  DefaultBool isPureOnly;
 
----------------
Variables should start with capital letters. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:40
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  bool isCalledbyCtor(const CallExpr *CE,ProgramStateRef state,const LocationContext *LCtx) const;
+  bool isCalledbyDtor(const CallExpr *CE,ProgramStateRef state,const LocationContext *LCtx) const;
----------------
The names of the arguments should start with an uppercase letter. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:72
+//GDM (generic data map) to determine if a function is called by an object
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectFlag, unsigned)
+//GDM (generic data map) to the memregion of this for the ctor and dtor
----------------
Do you need these traits above after having the maps bellow? 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:96
+ 
+  if((!CD && !DD) || (Ctorflag!=TrackedCtorDtorFlag && 
+      Dtorflag!=TrackedCtorDtorFlag)) 
----------------
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. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:129
 
-void WalkAST::VisitChildren(Stmt *S) {
-  for (Stmt *Child : S->children())
-    if (Child)
-      Visit(Child);
-}
+  const auto *CC = dyn_cast_or_null<CXXConstructorCall>(&Call);
+  const auto *CD = dyn_cast_or_null<CXXDestructorCall>(&Call);
----------------
I think you do not need the or_null suffix here since the argument of this cast will never be null. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:141
+  // Enter a constructor, increase the corresponding integer
+  if (dyn_cast<CXXConstructorDecl>(D)) {
+    unsigned Constructorflag = State->get<ConstructorFlag>();
----------------
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. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:168
     // this as a non-virtual call and do not warn.
-    if (CME->getQualifier())
-      callIsNonVirtual = true;
+    if (Expr *Base = CME->getBase()->IgnoreImpCasts()) {
+      if (!isa<CXXThisExpr>(Base)) {
----------------
Shouldn't this be const?


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:192
+      }
+      else {
+      BT_CT.reset(new BugType(this, 
----------------
In LLVM we do not write the braces for conditionals that guarding a single statement. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:218
+    }
+    ExplodedNode *N = C.generateNonFatalErrorNode();
+    auto Reporter = llvm::make_unique<BugReport>(*BT_DT, BT_DT->getName(), N);
----------------
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. 


================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:314
+      return None;
+    const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(FD->getParent());
+    if (!MD)
----------------
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. 



================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:332
+// Check the base of the callexpr is equal to the this of the ctor
+bool VirtualCallChecker::isCalledbyCtor(const CallExpr *CE,ProgramStateRef State,const LocationContext *LCtx) const {
+  if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
----------------
Maybe instead of callExpr, you should get CXXInstanceCall, which has a getCXXThisVal method. 


https://reviews.llvm.org/D34275