[clang] [NFCI][analyzer] invalidateRegions: require explicit State (PR #164434)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

<details>
<summary>Changes</summary>

The method `CallEvent::invalidateRegions()` takes (an unsigned `BlockCount` and) a `ProgramStateRef` which was previously defaulted to `nullptr` -- and when the state argument was `nullptr`, the method used the "inner" state of the `CallEvent` as a starting point for the invalidation.

My recent commit 0f6f13bdccd3345522b7d38294a72b802b6ffc28 turned the "inner" state of the `CallEvent` into a hidden `protected` implementation detail; so this commit follows that direction by removing the "defaults to the inner state" behavior from `invalidateRegions` to avoid exposing the inner state through this channel.

The method `CallEvent::invalidateRegions()` was only called in two locations, only one of those was relying on the existence of the default argument value, and even there it was really easy to explicitly pass the `State` object.

This commit also eliminates a footgun: with the old logic, if some code had tried to pass a state explicitly (presumably because the "inner" state of the call was obsolete) but that "new" state happened to be `nullptr`, then `invalidateRegions` would have silently used the inner state attached to the call. However, I reviewed the call sites and don't think that it was actually possible to reach this buggy execution path.

---
Full diff: https://github.com/llvm/llvm-project/pull/164434.diff


3 Files Affected:

- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h (+3-5) 
- (modified) clang/lib/StaticAnalyzer/Core/CallEvent.cpp (+5-7) 
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+1-1) 


``````````diff
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 4aee16576ebd1..66da79970ca19 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -372,12 +372,10 @@ class CallEvent {
   ProgramPoint getProgramPoint(bool IsPreVisit = false,
                                const ProgramPointTag *Tag = nullptr) const;
 
-  /// Returns a new state with all argument regions invalidated.
-  ///
-  /// This accepts an alternate state in case some processing has already
-  /// occurred.
+  /// Invalidates the regions (arguments, globals, special regions like 'this')
+  /// that may have been written by this call, returning the updated state.
   ProgramStateRef invalidateRegions(unsigned BlockCount,
-                                    ProgramStateRef Orig = nullptr) const;
+                                    ProgramStateRef State) const;
 
   using FrameBindingTy = std::pair<SVal, SVal>;
   using BindingsTy = SmallVectorImpl<FrameBindingTy>;
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 62460cc6f5b19..7a9a0bc673378 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -230,13 +230,11 @@ static void findPtrToConstParams(llvm::SmallSet<unsigned, 4> &PreserveArgs,
 }
 
 ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
-                                             ProgramStateRef Orig) const {
-  ProgramStateRef Result = (Orig ? Orig : getState());
-
+                                             ProgramStateRef State) const {
   // Don't invalidate anything if the callee is marked pure/const.
-  if (const Decl *callee = getDecl())
-    if (callee->hasAttr<PureAttr>() || callee->hasAttr<ConstAttr>())
-      return Result;
+  if (const Decl *Callee = getDecl())
+    if (Callee->hasAttr<PureAttr>() || Callee->hasAttr<ConstAttr>())
+      return State;
 
   SmallVector<SVal, 8> ValuesToInvalidate;
   RegionAndSymbolInvalidationTraits ETraits;
@@ -278,7 +276,7 @@ ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
   // Invalidate designated regions using the batch invalidation API.
   // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate
   //  global variables.
-  return Result->invalidateRegions(ValuesToInvalidate, getCFGElementRef(),
+  return State->invalidateRegions(ValuesToInvalidate, getCFGElementRef(),
                                    BlockCount, getLocationContext(),
                                    /*CausedByPointerEscape*/ true,
                                    /*Symbols=*/nullptr, this, &ETraits);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 75d7e265af0f3..00e3ef8311919 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1013,7 +1013,7 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
     // FIXME: Once we figure out how we want allocators to work,
     // we should be using the usual pre-/(default-)eval-/post-call checkers
     // here.
-    State = Call->invalidateRegions(blockCount);
+    State = Call->invalidateRegions(blockCount, State);
     if (!State)
       return;
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/164434