[NFCI][analyzer] invalidateRegions: require explicit State #164434
Conversation
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 0f6f13b 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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThe method My recent commit 0f6f13b turned the "inner" state of the The method 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 Full diff: https://github.com/llvm/llvm-project/pull/164434.diff 3 Files Affected:
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;
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThe method My recent commit 0f6f13b turned the "inner" state of the The method 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 Full diff: https://github.com/llvm/llvm-project/pull/164434.diff 3 Files Affected:
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;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
2 participants
The method
CallEvent::invalidateRegions()takes (an unsignedBlockCountand) aProgramStateRefwhich was previously defaulted tonullptr-- and when the state argument wasnullptr, the method used the "inner" state of theCallEventas a starting point for the invalidation.My recent commit 0f6f13b turned the "inner" state of the
CallEventinto a hiddenprotectedimplementation detail; so this commit follows that direction by removing the "defaults to the inner state" behavior frominvalidateRegionsto 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 theStateobject.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, theninvalidateRegionswould 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.