=?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]>, =?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 Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> This commit refactors `ExprEngine::evalBind` to eliminate the use of a `NodeBuilder` and fix incorrect logic that was apparently introduced because the `NodeBuilder` had obfuscated the underlying set operations. In the special case when the engine is binding to an `Unknown` or `Undefined` (memory) location, the old code invoked _either_ the `check::Bind` checkers _or_ the pointer escape checkers on each execution path. This commit ensures that on each execution path _both_ the `check::Bind` checkers _and then_ the pointer escape checkers get a chance to activate. I'm pretty sure that the bad logic did not cause incorrect behavior of the analyzer, because there are no `checkBind` checkers that generate non-sink transitions when the location is `Unknown` or `Undefined`. I also added an assertion that the location argument of `evalBind` cannot be a `NonLoc`, because this is a common sense precondition, seems to be actually true and makes it easier to reason about the behavior of this function. --- Full diff: https://github.com/llvm/llvm-project/pull/196313.diff 1 Files Affected: - (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+20-31) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index a7a503cd00ab0..fefef1cb06074 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3721,52 +3721,41 @@ ExprEngine::notifyCheckersOfPointerEscape(ProgramStateRef State, } /// evalBind - Handle the semantics of binding a value to a specific location. -/// This method is used by evalStore and (soon) VisitDeclStmt, and others. +/// This method is used by evalStore, VisitDeclStmt, and others. void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, - ExplodedNode *Pred, SVal location, SVal Val, + ExplodedNode *Pred, SVal Location, SVal Val, bool AtDeclInit, const ProgramPoint *PP) { + // It may be a Loc, UnknownVal or perhaps UndefinedVal. + assert(!isa<NonLoc>(Location) && "evalBind location should not be NonLoc!"); + const LocationContext *LC = Pred->getLocationContext(); - PostStmt PS(StoreE, LC); + PostStmt DefaultPP(StoreE, LC); if (!PP) - PP = &PS; + PP = &DefaultPP; // Do a previsit of the bind. ExplodedNodeSet CheckedSet; - getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val, + getCheckerManager().runCheckersForBind(CheckedSet, Pred, Location, Val, StoreE, AtDeclInit, *this, *PP); - NodeBuilder Bldr(CheckedSet, Dst, *currBldrCtx); - - // If the location is not a 'Loc', it will already be handled by - // the checkers. There is nothing left to do. - if (!isa<Loc>(location)) { - const ProgramPoint L = PostStore(StoreE, LC, /*Loc*/nullptr, - /*tag*/nullptr); - ProgramStateRef state = Pred->getState(); - state = processPointerEscapedOnBind(state, location, Val, LC); - Bldr.generateNode(L, state, Pred); - return; - } + for (ExplodedNode *PredI : CheckedSet) { + ProgramStateRef State = PredI->getState(); - for (const auto PredI : CheckedSet) { - ProgramStateRef state = PredI->getState(); + State = processPointerEscapedOnBind(State, Location, Val, LC); - state = processPointerEscapedOnBind(state, location, Val, LC); - - // When binding the value, pass on the hint that this is a initialization. - // For initializations, we do not need to inform clients of region - // changes. - state = state->bindLoc(location.castAs<Loc>(), Val, LC, - /* notifyChanges = */ !AtDeclInit); + if (auto AsLoc = Location.getAs<Loc>()) { + // When binding the value, pass on the hint that this is a + // initialization. For initializations, we do not need to inform clients + // of region changes. + State = State->bindLoc(*AsLoc, Val, LC, /*notifyChanges=*/!AtDeclInit); + } const MemRegion *LocReg = nullptr; - if (std::optional<loc::MemRegionVal> LocRegVal = - location.getAs<loc::MemRegionVal>()) { + if (auto LocRegVal = Location.getAs<loc::MemRegionVal>()) LocReg = LocRegVal->getRegion(); - } - const ProgramPoint L = PostStore(StoreE, LC, LocReg, nullptr); - Bldr.generateNode(L, state, PredI); + PostStore PS(StoreE, LC, LocReg, /*tag=*/nullptr); + Dst.insert(Engine.makeNode(PS, State, PredI)); } } `````````` </details> https://github.com/llvm/llvm-project/pull/196313 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
