-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[NFCI][analyzer] Make CallEvent::getState protected #162673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
`CallEvent` instances have a reference to a state object instead having separate data members for storing the arguments (as `SVal` instances), the return value (as `SVal`, if available), the dynamic type information and similar things. Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the `CallEvent` or through the `CheckerContext`. This redundancy is inelegant and bugprone (e.g. the recent commit llvm#160707 fixed a situation where the state attached to the `CallEvent` could be obsolete in `EvalCall` and `PointerEscape` callbacks), so this commit limits access to the state attached to a `CallEvent` and turns it into a protected implementation detail. In the future it may be a good idea to completely remove the state instance from the `CallEvent` and explicitly store the few parts of the state which are relevant for the call. In theory this commit should be a non-functional change (because AFAIK the `CallEvent` and `CheckerContext` provide the same state after the recent fix), but there is a small chance that it fixes some bugs that I do not know about.
This is intended as a follow-up commit after #160707 and was inspired by ideas within the discussion of that PR. |
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) Changes
Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the In the future it may be a good idea to completely remove the state instance from the In theory this commit should be a non-functional change (because AFAIK the Full diff: https://github.com/llvm/llvm-project/pull/162673.diff 6 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 5dcf03f7a4648..2b082d07da71a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -211,6 +211,16 @@ class CallEvent {
getExtraInvalidatedValues(ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const {}
+ /// The state in which the call is being evaluated.
+ /// CallEvent instances have a reference to a state object instead of storing
+ /// the argument `SVal`s, the return value (if available), the dynamic type
+ /// information and similar things as separate data members. However, the
+ /// existence of this state object is an implementation detail (and perhaps
+ /// it should be eventually eliminated), so use of this method should be
+ /// avoided if possible. (The checker callbacks can and should access the
+ /// canonical state through the CheckerContext.)
+ const ProgramStateRef &getState() const { return State; }
+
public:
CallEvent &operator=(const CallEvent &) = delete;
virtual ~CallEvent() = default;
@@ -231,8 +241,14 @@ class CallEvent {
}
void setForeign(bool B) const { Foreign = B; }
- /// The state in which the call is being evaluated.
- const ProgramStateRef &getState() const { return State; }
+ /// Returns the ASTContext which is (indirectly) associated with this call
+ /// event. This method is exposed for the convenience of a few checkers that
+ /// need the AST context in functions that take a CallEvent as argument. For
+ /// the sake of clarity prefer to get the ASTContext from more "natural"
+ /// sources (e.g. the CheckerContext) instead of using this method!
+ ASTContext &getASTContext() const {
+ return getState()->getStateManager().getContext();
+ }
/// The context in which the call is being evaluated.
const LocationContext *getLocationContext() const { return LCtx; }
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index bf35bee70870b..3ddd6590fcbb0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -104,7 +104,7 @@ class RAIIMutexDescriptor {
// this function is called instead of early returning it. To avoid this, a
// bool variable (IdentifierInfoInitialized) is used and the function will
// be run only once.
- const auto &ASTCtx = Call.getState()->getStateManager().getContext();
+ const auto &ASTCtx = Call.getASTContext();
Guard = &ASTCtx.Idents.get(GuardName);
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
index 9d3aeff465ca1..242084876a3c5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -929,7 +929,7 @@ ObjCDeallocChecker::getValueReleasedByNillingOut(const ObjCMethodCall &M,
SVal Arg = M.getArgSVal(0);
ProgramStateRef notNilState, nilState;
std::tie(notNilState, nilState) =
- M.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
+ C.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
if (!(nilState && !notNilState))
return nullptr;
diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
index f984caf59afb8..c1550f28f6f27 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
@@ -230,7 +230,7 @@ ObjCSuperDeallocChecker::isSuperDeallocMessage(const ObjCMethodCall &M) const {
if (M.getOriginExpr()->getReceiverKind() != ObjCMessageExpr::SuperInstance)
return false;
- ASTContext &Ctx = M.getState()->getStateManager().getContext();
+ ASTContext &Ctx = M.getASTContext();
initIdentifierInfoAndSelectors(Ctx);
return M.getSelector() == SELdealloc;
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
index 4fc1c576a9687..db8bbee8761d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -211,13 +211,13 @@ class StdVariantChecker : public Checker<eval::Call, check::RegionChanges> {
if (!DefaultType)
return;
- ProgramStateRef State = ConstructorCall->getState();
+ ProgramStateRef State = C.getState();
State = State->set<VariantHeldTypeMap>(ThisMemRegion, *DefaultType);
C.addTransition(State);
}
bool handleStdGetCall(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef State = Call.getState();
+ ProgramStateRef State = C.getState();
const auto &ArgType = Call.getArgSVal(0)
.getType(C.getASTContext())
diff --git a/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h b/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
index dec461296fed5..b8fb57213fd65 100644
--- a/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
+++ b/clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
@@ -52,7 +52,7 @@ removeInformationStoredForDeadInstances(const CallEvent &Call,
template <class TypeMap>
void handleConstructorAndAssignment(const CallEvent &Call, CheckerContext &C,
SVal ThisSVal) {
- ProgramStateRef State = Call.getState();
+ ProgramStateRef State = C.getState();
if (!State)
return;
|
CallEvent
instances have a reference to a state object instead having separate data members for storing the arguments (asSVal
instances), the return value (asSVal
, if available), the dynamic type information and similar things.Previously this state was publicly available, which meant that many checker callbacks had two ways to access the state: either through the
CallEvent
or through theCheckerContext
. This redundancy is inelegant and bugprone (e.g. the recent commit #160707 fixed a situation where the state attached to theCallEvent
could be obsolete inEvalCall
andPointerEscape
callbacks), so this commit limits access to the state attached to aCallEvent
and turns it into a protected implementation detail.In the future it may be a good idea to completely remove the state instance from the
CallEvent
and explicitly store the few parts of the state which are relevant for the call.In theory this commit should be a non-functional change (because AFAIK the
CallEvent
andCheckerContext
provide the same state after the recent fix), but there is a small chance that it fixes some bugs that I do not know about.