=?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
llvmorg-github-actions[bot] wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> Remove NodeBuilder use from `ExprEngine::Visit` to reduce its size from 700 lines to 600 lines of source code. This commit also moves the "for instance method operators, make sure the 'this' argument has a valid region" logic (~10 lines) from the huge `ExprEngine::Visit` to the more specific `VisitCallExpr`; and applies a few other very minor code quality improvements. ------ Note for reviewers: "why is this NFC" justifications can be found in the descriptions of the individual commits. --- Patch is 20.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/200837.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+31-162) - (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (+20) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index fcd61cee34fe1..34ca88705cba6 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1664,11 +1664,9 @@ ProgramStateRef ExprEngine::escapeValues(ProgramStateRef State, } void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, - ExplodedNodeSet &DstTop) { + ExplodedNodeSet &Dst) { PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(), S->getBeginLoc(), "Error evaluating statement"); - ExplodedNodeSet Dst; - NodeBuilder Bldr(Pred, DstTop, *currBldrCtx); assert(!isa<Expr>(S) || S == cast<Expr>(S)->IgnoreParens()); @@ -1800,8 +1798,9 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, case Stmt::OMPUnrollDirectiveClass: case Stmt::OMPMetaDirectiveClass: case Stmt::HLSLOutArgExprClass: { - const ExplodedNode *node = Bldr.generateSink(S, Pred, Pred->getState()); - Engine.addAbortedBlock(node, getCurrBlock()); + const ExplodedNode *Node = Engine.makePostStmtNode( + S, Pred->getState(), Pred, /*MarkAsSink=*/true); + Engine.addAbortedBlock(Node, getCurrBlock()); break; } @@ -1842,40 +1841,32 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, case Stmt::GNUNullExprClass: { // GNU __null is a pointer-width integer, not an actual pointer. - ProgramStateRef state = Pred->getState(); - state = state->BindExpr( - cast<Expr>(S), Pred->getStackFrame(), - svalBuilder.makeIntValWithWidth(getContext().VoidPtrTy, 0)); - Bldr.generateNode(S, Pred, state); + SVal Val = svalBuilder.makeIntValWithWidth(getContext().VoidPtrTy, 0); + Dst.insert(Engine.makeNodeWithBinding(Pred, cast<Expr>(S), Val)); break; } case Stmt::ObjCAtSynchronizedStmtClass: - Bldr.takeNodes(Pred); VisitObjCAtSynchronizedStmt(cast<ObjCAtSynchronizedStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Expr::ConstantExprClass: case Stmt::ExprWithCleanupsClass: + Dst.insert(Pred); // Handled due to fully linearised CFG. break; case Stmt::CXXBindTemporaryExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); ExplodedNodeSet Next; VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), PreVisit, Next); getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::ArrayInitLoopExprClass: - Bldr.takeNodes(Pred); VisitArrayInitLoopExpr(cast<ArrayInitLoopExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; // Cases not handled yet; but will handle some day. case Stmt::DesignatedInitExprClass: @@ -1931,29 +1922,23 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, case Stmt::SYCLUniqueStableNameExprClass: case Stmt::OpenACCAsteriskSizeExprClass: case Stmt::TypeTraitExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet preVisit; getCheckerManager().runCheckersForPreStmt(preVisit, Pred, S, *this); getCheckerManager().runCheckersForPostStmt(Dst, preVisit, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::AttributedStmtClass: { - Bldr.takeNodes(Pred); VisitAttributedStmt(cast<AttributedStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::CXXDefaultArgExprClass: case Stmt::CXXDefaultInitExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); ExplodedNodeSet Tmp; - NodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx); const Expr *ArgE; if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S)) @@ -1980,11 +1965,10 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, if (IsTemporary) State = createTemporaryRegionIfNeeded(State, SF, cast<Expr>(S), cast<Expr>(S)); - Bldr2.generateNode(S, I, State); + Tmp.insert(Engine.makePostStmtNode(S, State, I)); } getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this); - Bldr.addNodes(Dst); break; } @@ -1993,13 +1977,10 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, case Expr::ObjCArrayLiteralClass: case Expr::ObjCDictionaryLiteralClass: case Expr::ObjCBoxedExprClass: { - Bldr.takeNodes(Pred); - ExplodedNodeSet preVisit; getCheckerManager().runCheckersForPreStmt(preVisit, Pred, S, *this); ExplodedNodeSet Tmp; - NodeBuilder Bldr2(preVisit, Tmp, *currBldrCtx); const auto *Ex = cast<Expr>(S); QualType resultType = Ex->getType(); @@ -2023,18 +2004,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, State = escapeValues(State, Val, PSK_EscapeOther); } - Bldr2.generateNode(S, N, State); + Tmp.insert(Engine.makePostStmtNode(S, State, N)); } getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::ArraySubscriptExprClass: - Bldr.takeNodes(Pred); VisitArraySubscriptExpr(cast<ArraySubscriptExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::MatrixSingleSubscriptExprClass: @@ -2047,60 +2025,45 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, break; case Stmt::GCCAsmStmtClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); ExplodedNodeSet PostVisit; for (ExplodedNode *const N : PreVisit) VisitGCCAsmStmt(cast<GCCAsmStmt>(S), N, PostVisit); getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::MSAsmStmtClass: - Bldr.takeNodes(Pred); VisitMSAsmStmt(cast<MSAsmStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::BlockExprClass: - Bldr.takeNodes(Pred); VisitBlockExpr(cast<BlockExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::LambdaExprClass: if (AMgr.options.ShouldInlineLambdas) { - Bldr.takeNodes(Pred); VisitLambdaExpr(cast<LambdaExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); } else { - const ExplodedNode *node = Bldr.generateSink(S, Pred, Pred->getState()); - Engine.addAbortedBlock(node, getCurrBlock()); + const ExplodedNode *Node = Engine.makePostStmtNode( + S, Pred->getState(), Pred, /*MarkAsSink=*/true); + Engine.addAbortedBlock(Node, getCurrBlock()); } break; case Stmt::BinaryOperatorClass: { const auto *B = cast<BinaryOperator>(S); if (B->isLogicalOp()) { - Bldr.takeNodes(Pred); VisitLogicalExpr(B, Pred, Dst); - Bldr.addNodes(Dst); break; - } - else if (B->getOpcode() == BO_Comma) { - ProgramStateRef state = Pred->getState(); - Bldr.generateNode( - B, Pred, - state->BindExpr( - B, Pred->getStackFrame(), - state->getSVal(B->getRHS(), Pred->getStackFrame()))); + } else if (B->getOpcode() == BO_Comma) { + SVal Val = + Pred->getState()->getSVal(B->getRHS(), Pred->getStackFrame()); + Dst.insert(Engine.makeNodeWithBinding(Pred, B, Val)); break; } - Bldr.takeNodes(Pred); - if (AMgr.options.ShouldEagerlyAssume && (B->isRelationalOp() || B->isEqualityOp())) { ExplodedNodeSet Tmp; @@ -2110,64 +2073,31 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, else VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Dst); - Bldr.addNodes(Dst); break; } - case Stmt::CXXOperatorCallExprClass: { - const auto *OCE = cast<CXXOperatorCallExpr>(S); - - // For instance method operators, make sure the 'this' argument has a - // valid region. - const Decl *Callee = OCE->getCalleeDecl(); - if (const auto *MD = dyn_cast_or_null<CXXMethodDecl>(Callee)) { - if (MD->isImplicitObjectMemberFunction()) { - ProgramStateRef State = Pred->getState(); - const StackFrame *SF = Pred->getStackFrame(); - ProgramStateRef NewState = - createTemporaryRegionIfNeeded(State, SF, OCE->getArg(0)); - if (NewState != State) { - Pred = Bldr.generateNode(OCE, Pred, NewState, /*tag=*/nullptr, - ProgramPoint::PreStmtKind); - // Did we cache out? - if (!Pred) - break; - } - } - } - [[fallthrough]]; - } - + case Stmt::CXXOperatorCallExprClass: case Stmt::CallExprClass: case Stmt::CXXMemberCallExprClass: case Stmt::UserDefinedLiteralClass: - Bldr.takeNodes(Pred); VisitCallExpr(cast<CallExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::CXXCatchStmtClass: - Bldr.takeNodes(Pred); VisitCXXCatchStmt(cast<CXXCatchStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::CXXTemporaryObjectExprClass: case Stmt::CXXConstructExprClass: - Bldr.takeNodes(Pred); VisitCXXConstructExpr(cast<CXXConstructExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::CXXInheritedCtorInitExprClass: - Bldr.takeNodes(Pred); VisitCXXInheritedCtorInitExpr(cast<CXXInheritedCtorInitExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::CXXNewExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); @@ -2177,12 +2107,10 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, VisitCXXNewExpr(cast<CXXNewExpr>(S), i, PostVisit); getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::CXXDeleteExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; const auto *CDE = cast<CXXDeleteExpr>(S); getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); @@ -2192,59 +2120,44 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, for (const auto i : PostVisit) VisitCXXDeleteExpr(CDE, i, Dst); - Bldr.addNodes(Dst); break; } // FIXME: ChooseExpr is really a constant. We need to fix // the CFG do not model them as explicit control-flow. case Stmt::ChooseExprClass: { // __builtin_choose_expr - Bldr.takeNodes(Pred); const auto *C = cast<ChooseExpr>(S); VisitGuardedExpr(C, C->getLHS(), C->getRHS(), Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::CompoundAssignOperatorClass: - Bldr.takeNodes(Pred); VisitBinaryOperator(cast<BinaryOperator>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::CompoundLiteralExprClass: - Bldr.takeNodes(Pred); VisitCompoundLiteralExpr(cast<CompoundLiteralExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::BinaryConditionalOperatorClass: case Stmt::ConditionalOperatorClass: { // '?' operator - Bldr.takeNodes(Pred); const auto *C = cast<AbstractConditionalOperator>(S); VisitGuardedExpr(C, C->getTrueExpr(), C->getFalseExpr(), Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::CXXThisExprClass: - Bldr.takeNodes(Pred); VisitCXXThisExpr(cast<CXXThisExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::DeclRefExprClass: { - Bldr.takeNodes(Pred); const auto *DE = cast<DeclRefExpr>(S); VisitCommonDeclRefExpr(DE, DE->getDecl(), Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::DeclStmtClass: - Bldr.takeNodes(Pred); VisitDeclStmt(cast<DeclStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::ImplicitCastExprClass: @@ -2257,19 +2170,16 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, case Stmt::BuiltinBitCastExprClass: case Stmt::ObjCBridgedCastExprClass: case Stmt::CXXAddrspaceCastExprClass: { - Bldr.takeNodes(Pred); const auto *C = cast<CastExpr>(S); ExplodedNodeSet dstExpr; VisitCast(C, C->getSubExpr(), Pred, dstExpr); // Handle the postvisit checks. getCheckerManager().runCheckersForPostStmt(Dst, dstExpr, C, *this); - Bldr.addNodes(Dst); break; } case Expr::MaterializeTemporaryExprClass: { - Bldr.takeNodes(Pred); const auto *MTE = cast<MaterializeTemporaryExpr>(S); ExplodedNodeSet dstPrevisit; getCheckerManager().runCheckersForPreStmt(dstPrevisit, Pred, MTE, *this); @@ -2277,72 +2187,54 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, for (const auto i : dstPrevisit) CreateCXXTemporaryObject(MTE, i, dstExpr); getCheckerManager().runCheckersForPostStmt(Dst, dstExpr, MTE, *this); - Bldr.addNodes(Dst); break; } case Stmt::InitListExprClass: { const InitListExpr *E = cast<InitListExpr>(S); - Bldr.takeNodes(Pred); ConstructInitList(E, E->inits(), E->isTransparent(), Pred, Dst); - Bldr.addNodes(Dst); break; } case Expr::CXXParenListInitExprClass: { const CXXParenListInitExpr *E = cast<CXXParenListInitExpr>(S); - Bldr.takeNodes(Pred); ConstructInitList(E, E->getInitExprs(), /*IsTransparent*/ false, Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::MemberExprClass: - Bldr.takeNodes(Pred); VisitMemberExpr(cast<MemberExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::AtomicExprClass: - Bldr.takeNodes(Pred); VisitAtomicExpr(cast<AtomicExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::ObjCIvarRefExprClass: - Bldr.takeNodes(Pred); VisitLvalObjCIvarRefExpr(cast<ObjCIvarRefExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::ObjCForCollectionStmtClass: - Bldr.takeNodes(Pred); VisitObjCForCollectionStmt(cast<ObjCForCollectionStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::ObjCMessageExprClass: - Bldr.takeNodes(Pred); VisitObjCMessage(cast<ObjCMessageExpr>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::ObjCAtThrowStmtClass: case Stmt::CXXThrowExprClass: // FIXME: This is not complete. We basically treat @throw as // an abort. - Bldr.generateSink(S, Pred, Pred->getState()); + Engine.makePostStmtNode(S, Pred->getState(), Pred, /*MarkAsSink=*/true); break; case Stmt::ReturnStmtClass: - Bldr.takeNodes(Pred); VisitReturnStmt(cast<ReturnStmt>(S), Pred, Dst); - Bldr.addNodes(Dst); break; case Stmt::OffsetOfExprClass: { - Bldr.takeNodes(Pred); ExplodedNodeSet PreVisit; getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); @@ -2351,15 +2243,12 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, VisitOffsetOfExpr(cast<OffsetOfExpr>(S), Node, PostVisit); getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this); - Bldr.addNodes(Dst); break; } case Stmt::UnaryExprOrTypeTraitExprClass: - Bldr.takeNodes(Pred); - VisitUnaryExprOrTypeTraitExpr(cast<UnaryExprOrTypeTraitExpr>(S), - Pred, Dst); - Bldr.addNodes(Dst); + VisitUnaryExprOrTypeTraitExpr(cast<UnaryExprOrTypeTraitExpr>(S), Pred, + Dst); break; case Stmt::StmtExprClass: { @@ -2369,22 +2258,16 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, // Empty statement expression. assert(SE->getType() == getContext().VoidTy && "Empty statement expression must have void type."); - break; - } - - if (const auto *LastExpr = - dyn_cast<Expr>(*SE->getSubStmt()->body_rbegin())) { - ProgramStateRef state = Pred->getState(); - Bldr.generateNode( - SE, Pred, - state->BindExpr(SE, Pred->getStackFrame(), - state->getSVal(LastExpr, Pred->getStackFrame()))); + } else if (const auto *LastExpr = + dyn_cast<Expr>(*SE->getSubStmt()->body_rbegin())) { + SVal Val = Pred->getState()->getSVal(LastExpr, Pred->getStackFrame()); + Pred = Engine.makeNodeWithBinding(Pred, SE, Val); } + Dst.insert(Pred); break; } case Stmt::UnaryOperatorClass: { - Bldr.takeNodes(Pred); const auto *U = cast<UnaryOperator>(S); if (AMgr.options.ShouldEagerlyAssume && (U->getOpcode() == UO_LNot)) { ExplodedNodeSet Tmp; @@ -2393,25 +2276,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, } else VisitUnaryOperator(U, Pred, Dst); - Bldr.addNodes(Dst); break; } case Stmt::PseudoObjectExprClass: { - Bldr.takeNodes(Pred); - ProgramStateRef state = Pred->getState(); const auto *PE = cast<PseudoObjectExpr>(S); - if (const Expr *Result = PE->getResultExpr()) { - SVal V = state->getSVal(Result, Pred->getStackFrame()); - Bldr.generateNode( - S, Pred, state->BindExpr(cast<Expr>(S), Pred->getStackFrame(), V)); - } - else - Bldr.generateNode(S, Pred, - state->BindExpr(cast<Expr>(S), Pred->getStackFrame(), - UnknownVal())); - - Bldr.addNodes(Dst); + SVal V = UnknownVal(); + if (const Expr *Result = PE->getResultExpr()) + V = Pred->getState()->getSVal(Result, Pred->getStackFrame()); + Dst.insert(Engine.makeNodeWithBinding(Pred, PE, V)); break; } @@ -2419,14 +2292,10 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, // ObjCIndirectCopyRestoreExpr implies passing a temporary for // correctness of lifetime management. Due to limited analysis // of ARC, this is implemented as direct arg passing. - Bldr.takeNodes(Pred); - ProgramStateRef state = Pred->getState(); const auto *OIE = cast<ObjCIndirectCopyRestoreExpr>(S); const Expr *E = OIE->getSubExpr(); - SVal V = state->getSVal(E, Pred->getStackFrame()); - Bldr.generateNode( - S, Pred, state->BindExpr(cast<Expr>(S), Pred->getStackFrame(), V)); - Bldr.addNodes(Dst); + SVal V = Pred->getState()->getSVal(E, Pred->getStackFrame()); + Dst.insert(Engine.makeNodeWithBinding(Pred, OIE, V)); break; } } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 0c6d35876db2f..7ecca2af626f3 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -590,6 +590,26 @@ static ProgramStateRef getInlineFailedState(ProgramStateRef State, void ExprEngine::VisitCallExpr(const CallExpr *CE, ExplodedNode *Pred, ExplodedNodeSet &dst) { + if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(CE)) { + // For instance method operators, make sure the 'this' argument has a + // valid region. + // FIXME: Why is this only applied for operator calls and not other calls? + const Decl *Callee = OCE->getCalleeDecl(); + if (const auto *MD = dyn_cast_or_null<CXXMethodDecl>(Callee)) { + if (MD->isImplicitObjectMemberFunction()) { + ProgramStateRef State = Pred->getState(); + const StackFrame *SF = Pred->getStackFrame(); + ProgramStateRef NewSt... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/200837 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
