[clang] [analyzer] Clean up evalBind, fix bad logic (PR #196313)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/196313 at github.com>


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