https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/162673
`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 https://github.com/llvm/llvm-project/pull/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. From 769a30920e1202605cd3bd23f110211a13255ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 1 Oct 2025 17:02:36 +0200 Subject: [PATCH] [NFCI][analyzer] Make CallEvent::getState protected `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 https://github.com/llvm/llvm-project/pull/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. --- .../Core/PathSensitive/CallEvent.h | 20 +++++++++++++++++-- .../BlockInCriticalSectionChecker.cpp | 2 +- .../Checkers/CheckObjCDealloc.cpp | 2 +- .../Checkers/ObjCSuperDeallocChecker.cpp | 2 +- .../Checkers/StdVariantChecker.cpp | 4 ++-- .../Checkers/TaggedUnionModeling.h | 2 +- 6 files changed, 24 insertions(+), 8 deletions(-) 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; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
