Add PostStmt callback for ArraySubscriptExpr

[a.sidorin]

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

28

I'd like to see Stmt-related options together (PreStmt<Type1>, PostStmt<Type1>, PreStmt<Type2>, PostStmt<Type2>). Not sure, however. Anna, Artem, what do you think? Is there any style recommendation for that?

lib/StaticAnalyzer/Core/ExprEngine.cpp

1990

You should use a temporary node set for PreStmt and then use Dst for PostStmt. Please see an example in ExprEngine.cpp:1213.

[a.sidorin]

lib/StaticAnalyzer/Core/ExprEngine.cpp

1982

auto *Node

[a.sidorin]

Looks good. Anna/Artem?

[NoQ]

So we did run checkers for PreStmt but forgot PostStmt? Nice! Thanks for noticing and fixing.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

28–31

I agree that having one place to change stuff is better than having two places to change stuff, so i'd also prefer pre-post-pre-pos-...-pre-post. In the long term, when this list becomes very long, i'd still suggest we only make PreStmt<Stmt> and PostStmt<Stmt> and construct some dynamic switch in the implementation.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1982

Indent seems to have moved.

[zaks.anna]

Looks good overall.

Please, include a more elaborate commit message, mentioning that you are adding a new callback.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1974

Let's keep the prefix of the old name for "checkerPreStmt" as it is more explicit - those are the states we get by running the checkers registered for preStmt. This is also more consistent with naming used elsewhere.

(Not sure what to do about capitalization though. Looks like it's already inconsistent in this file. The LLVM coding style prefers lower letters, but this part of the analyzer has been written before that went into effect.)

[a.sidorin]

Seems like all comments were addressed. Thank you!

[zaks.anna]

Thank you!

Do you have commit access or should we commit?

[jan]

Please go ahead and commit. I do not have access.
Thanks

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>,
-                                             check::PostStmt<CastExpr>> {
+                                             check::PreStmt<ArraySubscriptExpr>,
+                                             check::PostStmt<CastExpr>,
+                                             check::PostStmt<ArraySubscriptExpr>> {
   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 checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr"))
+      llvm::errs() << "PreStmt<ArraySubscriptExpr>\n";
+  }
+
   void checkPostStmt(const CastExpr *CE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtCastExpr"))
       llvm::errs() << "PostStmt<CastExpr> (Kind : " << CE->getCastKindName()
                    << ")\n";
   }
+
+  void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
+      llvm::errs() << "PostStmt<ArraySubscriptExpr>\n";
+  }
 };
 }
 
 //===----------------------------------------------------------------------===//
 // Registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerAnalysisOrderChecker(CheckerManager &mgr) {
   mgr.registerChecker<AnalysisOrderChecker>();
 }

lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::VisitLvalArraySubscriptExpr(const ArraySubscriptExpr *A,
                                              ExplodedNodeSet &Dst){
 
   const Expr *Base = A->getBase()->IgnoreParens();
   const Expr *Idx  = A->getIdx()->IgnoreParens();
 
   ExplodedNodeSet checkerPreStmt;
   getCheckerManager().runCheckersForPreStmt(checkerPreStmt, Pred, A, *this);
 
   StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
   assert(A->isGLValue() ||
           (!AMgr.getLangOpts().CPlusPlus &&
            A->getType().isCForbiddenLValueType()));
 
   for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
                                  ei = checkerPreStmt.end(); it != ei; ++it) {
     const LocationContext *LCtx = (*it)->getLocationContext();
     ProgramStateRef state = (*it)->getState();
     SVal V = state->getLValue(A->getType(),
                               state->getSVal(Idx, LCtx),
                               state->getSVal(Base, LCtx));
     Bldr.generateNode(A, *it, state->BindExpr(A, LCtx, V), nullptr,
                       ProgramPoint::PostLValueKind);
   }
+
+  getCheckerManager().runCheckersForPostStmt(checkerPreStmt, Dst, A, *this);
+
 }
 
 /// VisitMemberExpr - Transfer function for member expressions.
 void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
                                  ExplodedNodeSet &Dst) {
 
   // FIXME: Prechecks eventually go in ::Visit().
   ExplodedNodeSet CheckedSet;

test/Analysis/arraysubscriptexpr-callback.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtArraySubscriptExpr=true,debug.AnalysisOrder:PostStmtArraySubscriptExpr=true %s 2>&1 | FileCheck %s
+
+
+char x[1];
+void test(char c) {
+  x[0] = c;
+}
+
+// CHECK: PreStmt<ArraySubscriptExpr>
+// CHECK-NEXT: PostStmt<ArraySubscriptExpr>