[analyzer] ExprEngine: don't launch PreStmt<CastExpr> twice

[a.sidorin]

Summary

CheckerManager::runCheckersForPreStmt() is called twice for CastExpr: first, in ExprEngine::Visit() and then in ExprEngine::VisitCast(). This patch removes the first check as redundant.

This patch lacks a test case. If you have any idea for a test case, please, share it :)

[zaks.anna]

LGTM. Thanks!

[NoQ]

Maybe we could extend CheckerDocumentationChecker to print stuff whenever any callback is called (since it's anyway subscribed on all callbacks in existence; or make a separate debug checker), and make tests that ensure that the callback order is what we expect.

It might lead to a few more interesting discoveries.

[a.sidorin]

I like the idea of a separate debug checker like AnalysisCallbackOrder. Will try to implement.

[a.sidorin]

Add a test checker and a test to ensure that patch works as expected.

[NoQ]

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

25 ↗

(On Diff #69366)

Not sure we need those.

32 ↗

(On Diff #69366)

Hmm, you aren't looking for the easy path, are you :) That might lead to a huge visitor-like macro magic if you want to turn particular statement kind groups on and off.

I think it's not scary to enable all callbacks at once. Maybe print statement kind in each callback. The only outcome of this would be ... more tests! Which doesn't sound that bad.

[a.sidorin]

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

32 ↗

(On Diff #69366)

Hi! I was not going for "-enable/-disable" style. The target of this is to keep regression tests as easy as possible. Otherwise, we may need to change every test after adding a callback. This may also make tests to test not what they need to test and I don't like it. I may add '*'-mode, but if somebody needs to see all callbacks fired, ViewExplodedGraph is much better tool to use.

[a.sidorin]

Remove namespace usage; implement a '*'-mode to print all callbacks.

[NoQ]

Cool!

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

def LLVMConventionsChecker : Checker<"Conventions">,
   DescFile<"LLVMConventionsChecker.cpp">;
 
 //===----------------------------------------------------------------------===//
 // Debugging checkers (for analyzer development).
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Debug in {
 
+def AnalysisOrderChecker : Checker<"AnalysisOrder">,
+  HelpText<"Print callbacks that are called during analysis in order">,
+  DescFile<"AnalysisOrder.cpp">;
+
 def DominatorsTreeDumper : Checker<"DumpDominators">,
   HelpText<"Print the dominance tree for a given CFG">,
   DescFile<"DebugCheckers.cpp">;
 
 def LiveVariablesDumper : Checker<"DumpLiveVars">,
   HelpText<"Print results of live variable analysis">,
   DescFile<"DebugCheckers.cpp">;
 

cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

+//===- AnalysisOrderChecker - Print callbacks called ------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This checker prints callbacks that are called during analysis.
+// This is required to ensure that callbacks are fired in order
+// and do not duplicate or get lost.
+// Feel free to extend this checker with any callback you need to check.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>,
+                                             check::PostStmt<CastExpr>> {
+  bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
+    AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
+    return Opts.getBooleanOption("*", false, this) ||
+        Opts.getBooleanOption(CallbackName, false, this);
+  }
+
+public:
+  void checkPreStmt(const CastExpr *CE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreStmtCastExpr"))
+      llvm::errs() << "PreStmt<CastExpr> (Kind : " << CE->getCastKindName()
+                   << ")\n";
+  }
+
+  void checkPostStmt(const CastExpr *CE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostStmtCastExpr"))
+      llvm::errs() << "PostStmt<CastExpr> (Kind : " << CE->getCastKindName()
+                   << ")\n";
+  }
+};
+}
+
+//===----------------------------------------------------------------------===//
+// Registration.
+//===----------------------------------------------------------------------===//
+
+void ento::registerAnalysisOrderChecker(CheckerManager &mgr) {
+  mgr.registerChecker<AnalysisOrderChecker>();
+}

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangStaticAnalyzerCheckers
   AllocationDiagnostics.cpp
+  AnalysisOrderChecker.cpp
   AnalyzerStatsChecker.cpp
   ArrayBoundChecker.cpp
   ArrayBoundCheckerV2.cpp
   BasicObjCFoundationChecks.cpp
   BoolAssignmentChecker.cpp
   BuiltinFunctionChecker.cpp
   CStringChecker.cpp
   CStringSyntaxChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

switch (S->getStmtClass()) {
     case Stmt::CXXStaticCastExprClass:
     case Stmt::CXXDynamicCastExprClass:
     case Stmt::CXXReinterpretCastExprClass:
     case Stmt::CXXConstCastExprClass:
     case Stmt::CXXFunctionalCastExprClass:
     case Stmt::ObjCBridgedCastExprClass: {
       Bldr.takeNodes(Pred);
       const CastExpr *C = cast<CastExpr>(S);
-      // Handle the previsit checks.
-      ExplodedNodeSet dstPrevisit;
-      getCheckerManager().runCheckersForPreStmt(dstPrevisit, Pred, C, *this);
-
-      // Handle the expression itself.
       ExplodedNodeSet dstExpr;
-      for (ExplodedNodeSet::iterator i = dstPrevisit.begin(),
-                                     e = dstPrevisit.end(); i != e ; ++i) {
-        VisitCast(C, C->getSubExpr(), *i, dstExpr);
-      }
+      VisitCast(C, C->getSubExpr(), Pred, dstExpr);
 
       // Handle the postvisit checks.
       getCheckerManager().runCheckersForPostStmt(Dst, dstExpr, C, *this);
       Bldr.addNodes(Dst);
       break;
     }
 
     case Expr::MaterializeTemporaryExprClass: {

cfe/trunk/test/Analysis/castexpr-callback.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtCastExpr=true,debug.AnalysisOrder:PostStmtCastExpr=true %s 2>&1 | FileCheck %s
+
+void test(char c) {
+  int i = (int)c;
+}
+
+// CHECK: PreStmt<CastExpr> (Kind : LValueToRValue)
+// CHECK-NEXT: PostStmt<CastExpr> (Kind : LValueToRValue)
+// CHECK-NEXT: PreStmt<CastExpr> (Kind : IntegralCast)
+// CHECK-NEXT: PostStmt<CastExpr> (Kind : IntegralCast)