[analyzer] Fix CXXNewExpr callback order.

[NoQ]

Summary

Even if we later change how these callbacks work (as in http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html), i wanted to clean up the current mess so that it at least worked in any reasonable manner.

1. Make the PreStmt<CXXNewExpr> callback actually work. It is called on CXXNewExpr pre-visit in ExprEngine.
2. In -analyzer-config c++-allocator-inlining, don't call the PostStmt<CXXNewExpr> callback twice when the allocator is inlined.

[NoQ]

test/Analysis/cxxnewexpr-callback-inline.cpp

31–32

This ensures that there are no other callbacks after PostStmt<CXXNewExpr>, in particular that there's no another PostStmt<CXXNewExpr>.

[dcoughlin]

Looks great. It is nice to have this fixed and cleaned up!

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

95

Style nit: use auto for the casts.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 // 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/AST/ExprCXX.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.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>,
                      check::PreStmt<ArraySubscriptExpr>,
                      check::PostStmt<ArraySubscriptExpr>,
+                     check::PreStmt<CXXNewExpr>,
+                     check::PostStmt<CXXNewExpr>,
+                     check::PreCall,
+                     check::PostCall,
+                     check::NewAllocator,
                      check::Bind,
                      check::RegionChanges> {
   bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
     return Opts.getBooleanOption("*", false, this) ||
         Opts.getBooleanOption(CallbackName, false, this);
   }
 
   bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
public:
   }
 
   void checkPostStmt(const ArraySubscriptExpr *SubExpr,
                      CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
       llvm::errs() << "PostStmt<ArraySubscriptExpr>\n";
   }
 
+  void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreStmtCXXNewExpr"))
+      llvm::errs() << "PreStmt<CXXNewExpr>\n";
+  }
+
+  void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostStmtCXXNewExpr"))
+      llvm::errs() << "PostStmt<CXXNewExpr>\n";
+  }
+
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreCall")) {
+      llvm::errs() << "PreCall";
+      if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
+        llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
+      llvm::errs() << '\n';
+    }
+  }
+
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostCall")) {
+      llvm::errs() << "PostCall";
+      if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
+        llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
+      llvm::errs() << '\n';
+    }
+  }
+
+  void checkNewAllocator(const CXXNewExpr *CNE, SVal Target,
+                         CheckerContext &C) const {
+    if (isCallbackEnabled(C, "NewAllocator"))
+      llvm::errs() << "NewAllocator\n";
+  }
+
   void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
     if (isCallbackEnabled(C, "Bind"))
       llvm::errs() << "Bind\n";
   }
 
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
                      const InvalidatedSymbols *Invalidated,

lib/StaticAnalyzer/Core/ExprEngine.cpp

case Stmt::CXXConstructExprClass: {
       Bldr.takeNodes(Pred);
       VisitCXXConstructExpr(cast<CXXConstructExpr>(S), Pred, Dst);
       Bldr.addNodes(Dst);
       break;
     }
 
     case Stmt::CXXNewExprClass: {
       Bldr.takeNodes(Pred);
+
+      ExplodedNodeSet PreVisit;
+      getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
       ExplodedNodeSet PostVisit;
-      VisitCXXNewExpr(cast<CXXNewExpr>(S), Pred, PostVisit);
+      for (ExplodedNodeSet::iterator i = PreVisit.begin(),
+                                     e = PreVisit.end(); i != e ; ++i) {
+        VisitCXXNewExpr(cast<CXXNewExpr>(S), *i, PostVisit);
+      }
+
       getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
       Bldr.addNodes(Dst);
       break;
     }
 
     case Stmt::CXXDeleteExprClass: {
       Bldr.takeNodes(Pred);
       ExplodedNodeSet PreVisit;

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (const CXXNewExpr *CNE = dyn_cast_or_null<CXXNewExpr>(CE)) {
                                                  *UpdatedCall, *this,
                                                  /*WasInlined=*/true);
     }
     ExplodedNodeSet Dst;
     if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(Call)) {
       getCheckerManager().runCheckersForPostObjCMessage(Dst, DstPostCall, *Msg,
                                                         *this,
                                                         /*WasInlined=*/true);
-    } else if (CE) {
+    } else if (CE &&
+               !(isa<CXXNewExpr>(CE) && // Called when visiting CXXNewExpr.
+                 AMgr.getAnalyzerOptions().mayInlineCXXAllocator())) {
       getCheckerManager().runCheckersForPostStmt(Dst, DstPostCall, CE,
                                                  *this, /*WasInlined=*/true);
     } else {
       Dst.insert(DstPostCall);
     }
 
     // Enqueue the next element in the block.
     for (ExplodedNodeSet::iterator PSI = Dst.begin(), PSE = Dst.end();

test/Analysis/cxxnewexpr-callback-inline.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=true,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+  void *malloc(size_t);
+}
+
+void *operator new(size_t size) { return std::malloc(size); }
+
+struct S {
+  S() {}
+};
+
+void foo();
+
+void test() {
+  S *s = new S();
+  foo();
+}
+
+// CHECK:      PreCall (operator new)
+// CHECK-NEXT: PreCall (std::malloc)
+// CHECK-NEXT: PostCall (std::malloc)
+// CHECK-NEXT: PostCall (operator new)
+// CHECK-NEXT: NewAllocator
+// CHECK-NEXT: PreCall (S::S)
+// CHECK-NEXT: PostCall (S::S)
+// CHECK-NEXT: PreStmt<CXXNewExpr>
+// CHECK-NEXT: PostStmt<CXXNewExpr>
+// CHECK-NEXT: PreCall (foo)
+// CHECK-NEXT: PostCall (foo)

test/Analysis/cxxnewexpr-callback-noinline.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+  void *malloc(size_t);
+}
+
+void *operator new(size_t size) { return std::malloc(size); }
+
+struct S {
+  S() {}
+};
+
+void foo();
+
+void test() {
+  S *s = new S();
+  foo();
+}
+
+// CHECK:      PreCall (S::S)
+// CHECK-NEXT: PostCall (S::S)
+// CHECK-NEXT: PreStmt<CXXNewExpr>
+// CHECK-NEXT: PostStmt<CXXNewExpr>
+// CHECK-NEXT: PreCall (foo)
+// CHECK-NEXT: PostCall (foo)
+// CHECK-NEXT: PreCall (std::malloc)
+// CHECK-NEXT: PostCall (std::malloc)