diff --git a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp index e64153d53bbd6..309e3d250de06 100644 --- a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp +++ b/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; } diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 392c7eeea234a..c71623575ae97 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/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 diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index dee34e3e9d6a5..75d7e265af0f3 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/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 { diff --git a/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp b/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp index 0e1ec2f9de566..743c5ad0fa8cd 100644 --- a/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp +++ b/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]