From 769a30920e1202605cd3bd23f110211a13255ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Wed, 1 Oct 2025 17:02:36 +0200 Subject: [PATCH 1/5] [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()); + C.getState()->assume(Arg.castAs()); 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 { if (!DefaultType) return; - ProgramStateRef State = ConstructorCall->getState(); + ProgramStateRef State = C.getState(); State = State->set(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 void handleConstructorAndAssignment(const CallEvent &Call, CheckerContext &C, SVal ThisSVal) { - ProgramStateRef State = Call.getState(); + ProgramStateRef State = C.getState(); if (!State) return; From e0d10bac7e9fe15da1e6b35be089369ca990902f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Mon, 13 Oct 2025 14:57:58 +0200 Subject: [PATCH 2/5] Shorten and clarify comments --- .../Core/PathSensitive/CallEvent.h | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h index 2b082d07da71a..f07506394e779 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -211,15 +211,14 @@ 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; } + /// A state for looking up relevant Environment entries (arguments, return + /// value), dynamic type information and similar "stable" things. + /// WARNING: During the evaluation of a function call, several state + /// transitions happen, so this state can become partially obsolete! + /// + /// TODO: Instead of storing a complete state object in the CallEvent, only + /// store the relevant parts (argument/return SVals, dynamic type etc.). + ProgramStateRef getState() const { return State; } public: CallEvent &operator=(const CallEvent &) = delete; @@ -241,11 +240,8 @@ class CallEvent { } void setForeign(bool B) const { Foreign = B; } - /// 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! + /// NOTE: There are plans for refactoring that would eliminate this method. + /// Prefer to use CheckerContext::getASTContext if possible! ASTContext &getASTContext() const { return getState()->getStateManager().getContext(); } From ca654161610f53bfad36c48dd0ae765c86c77b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Mon, 13 Oct 2025 15:39:15 +0200 Subject: [PATCH 3/5] Make the returned ASTContext const --- .../clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h | 2 +- .../lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h index f07506394e779..ef245227df999 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -242,7 +242,7 @@ class CallEvent { /// NOTE: There are plans for refactoring that would eliminate this method. /// Prefer to use CheckerContext::getASTContext if possible! - ASTContext &getASTContext() const { + const ASTContext &getASTContext() const { return getState()->getStateManager().getContext(); } diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index c1550f28f6f27..3f9600b0e9b70 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -34,7 +34,7 @@ class ObjCSuperDeallocChecker this, "[super dealloc] should not be called more than once", categories::CoreFoundationObjectiveC}; - void initIdentifierInfoAndSelectors(ASTContext &Ctx) const; + void initIdentifierInfoAndSelectors(const ASTContext &Ctx) const; bool isSuperDeallocMessage(const ObjCMethodCall &M) const; @@ -215,7 +215,7 @@ void ObjCSuperDeallocChecker::diagnoseCallArguments(const CallEvent &CE, } void -ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(ASTContext &Ctx) const { +ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(const ASTContext &Ctx) const { if (IIdealloc) return; @@ -230,7 +230,7 @@ ObjCSuperDeallocChecker::isSuperDeallocMessage(const ObjCMethodCall &M) const { if (M.getOriginExpr()->getReceiverKind() != ObjCMessageExpr::SuperInstance) return false; - ASTContext &Ctx = M.getASTContext(); + const ASTContext &Ctx = M.getASTContext(); initIdentifierInfoAndSelectors(Ctx); return M.getSelector() == SELdealloc; From 1d76d59df9ba7e5ee99826c358ddc5eac06f7b42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Mon, 13 Oct 2025 15:50:06 +0200 Subject: [PATCH 4/5] Satisfy git-clang-format --- clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index 3f9600b0e9b70..227cbfac770d2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -214,8 +214,8 @@ void ObjCSuperDeallocChecker::diagnoseCallArguments(const CallEvent &CE, } } -void -ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(const ASTContext &Ctx) const { +void ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors( + const ASTContext &Ctx) const { if (IIdealloc) return; From dd4a5c7caf38ba3aea8ad89159c067d6d79e0e78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Wed, 15 Oct 2025 11:35:23 +0200 Subject: [PATCH 5/5] Clarify TODO commet Co-authored-by: Artem Dergachev --- .../clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h index ef245227df999..3f26b30459a19 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -217,7 +217,8 @@ class CallEvent { /// transitions happen, so this state can become partially obsolete! /// /// TODO: Instead of storing a complete state object in the CallEvent, only - /// store the relevant parts (argument/return SVals, dynamic type etc.). + /// store the relevant parts (such as argument/return SVals etc.) that aren't + /// allowed to become obsolete until the end of the call evaluation. ProgramStateRef getState() const { return State; } public: