From 740ae4f37ecf3b59d6f5e1a4806db0f6147c746f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Tue, 21 Oct 2025 14:06:26 +0200 Subject: [PATCH 1/2] [NFCI][analyzer] invalidateRegions: require explicit State 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. --- .../StaticAnalyzer/Core/PathSensitive/CallEvent.h | 8 +++----- clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 12 +++++------- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-) 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; using BindingsTy = SmallVectorImpl; 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 &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() || callee->hasAttr()) - return Result; + if (const Decl *Callee = getDecl()) + if (Callee->hasAttr() || Callee->hasAttr()) + return State; SmallVector 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; From c5245b0a6e63fdd1a708e4bbecdb3a9685a24783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Tue, 21 Oct 2025 16:48:33 +0200 Subject: [PATCH 2/2] Satisfy git-clang-format --- clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp index 7a9a0bc673378..d04c827ce1391 100644 --- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -277,9 +277,9 @@ ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount, // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate // global variables. return State->invalidateRegions(ValuesToInvalidate, getCFGElementRef(), - BlockCount, getLocationContext(), - /*CausedByPointerEscape*/ true, - /*Symbols=*/nullptr, this, &ETraits); + BlockCount, getLocationContext(), + /*CausedByPointerEscape*/ true, + /*Symbols=*/nullptr, this, &ETraits); } ProgramPoint CallEvent::getProgramPoint(bool IsPreVisit,