[analyzer][NFC] Explain why operator new/delete should never be eval-called

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

@@ -129,7 +129,8 @@ class AnalysisOrderChecker
       llvm::errs() << " {argno: " << Call.getNumArgs() << '}';
       llvm::errs() << " [" << Call.getKindAsString() << ']';
       llvm::errs() << '\n';
-      return true;
+      // We can't return `true` from this callback without binding the return
+      // value. Let's just fallthrough here and return `false`.
     }
     return false;
   }

clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

@@ -262,6 +262,15 @@ class CheckerDocumentation
   /// state. This callback allows a checker to provide domain specific knowledge
   /// about the particular functions it knows about.
   ///
+  /// Note that to evaluate a call, the handler MUST bind the return value if
+  /// its a non-void function. Invalidate the arguments if necessary.
+  ///
+  /// Note that in general, user-provided functions should not be eval-called
+  /// because the checker can't predict the exact semantics/contract of the
+  /// callee, and by having the eval::Call callback, we also prevent it from
+  /// getting inlined, potentially regressing analysis quality.
+  /// Consider using check::PreCall or check::PostCall to allow inlining.
+  ///
   /// \returns true if the call has been successfully evaluated
   /// and false otherwise. Note, that only one checker can evaluate a call. If
   /// more than one checker claims that they can evaluate the same call the

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -909,7 +909,14 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
   for (ExplodedNode *I : DstPreCall) {
-    // FIXME: Provide evalCall for checkers?
+    // Operator new calls (CXXNewExpr) are intentionally not eval-called,
+    // because it does not make sense to eval-call user-provided functions.
+    // 1) If the new operator can be inlined, then don't prevent it from
+    //    inlining by having an eval-call of that operator.
+    // 2) If it can't be inlined, then the default conservative modeling
+    //    is what we want anyway.
+    // So the best is to not allow eval-calling CXXNewExprs from checkers.
+    // Checkers can provide their pre/post-call callbacks if needed.
     defaultEvalCall(CallBldr, I, *Call);
   }
   // If the call is inlined, DstPostCall will be empty and we bail out now.
@@ -1110,6 +1117,10 @@ void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
   if (AMgr.getAnalyzerOptions().MayInlineCXXAllocator) {
     StmtNodeBuilder Bldr(DstPreCall, DstPostCall, *currBldrCtx);
     for (ExplodedNode *I : DstPreCall) {
+      // Intentionally either inline or conservative eval-call the operator
+      // delete, but avoid triggering an eval-call event for checkers.
+      // As detailed at handling CXXNewExprs, in short, because it does not
+      // really make sense to eval-call user-provided functions.
       defaultEvalCall(Bldr, I, *Call);
     }
   } else {

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp

@@ -18,16 +18,33 @@ void foo() {
   C C0;
   C C1(42);
   C *C2 = new C{2, 3};
+  delete C2;
 }
 
 // CHECK:  PreCall (C::C) [CXXConstructorCall]
 // CHECK-NEXT:  EvalCall (C::C) {argno: 0} [CXXConstructorCall]
 // CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+
 // CHECK-NEXT:  PreCall (C::C) [CXXConstructorCall]
 // CHECK-NEXT:  EvalCall (C::C) {argno: 1} [CXXConstructorCall]
 // CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+
 // CHECK-NEXT:  PreCall (operator new) [CXXAllocatorCall]
+//    COMMENT: Operator new calls (CXXNewExpr) are intentionally not eval-called,
+//    COMMENT: because it does not make sense to eval call user-provided functions.
+//    COMMENT: 1) If the new operator can be inlined, then don't prevent it from
+//    COMMENT:    inlining by having an eval-call of that operator.
+//    COMMENT: 2) If it can't be inlined, then the default conservative modeling
+//    COMMENT:    is what we anyways want anyway.
+//    COMMENT: So the EvalCall event will not be triggered for operator new calls.
+// CHECK-NOT:   EvalCall
 // CHECK-NEXT:  PostCall (operator new) [CXXAllocatorCall]
+
 // CHECK-NEXT:  PreCall (C::C) [CXXConstructorCall]
 // CHECK-NEXT:  EvalCall (C::C) {argno: 2} [CXXConstructorCall]
 // CHECK-NEXT:  PostCall (C::C) [CXXConstructorCall]
+
+// CHECK-NEXT: PreCall (operator delete) [CXXDeallocatorCall]
+//    COMMENT: Same reasoning as for CXXNewExprs above.
+// CHECK-NOT:  EvalCall
+// CHECK-NEXT: PostCall (operator delete) [CXXDeallocatorCall]