[NFCI][analyzer] Make CallEvent::getState protected

[NagyDonat]

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 #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.

[NagyDonat]

This is intended as a follow-up commit after #160707 and was inspired by ideas within the discussion of that PR.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

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 #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.

---

Full diff: https://github.com/llvm/llvm-project/pull/162673.diff

6 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h (+18-2)
• (modified) clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp (+2-2)
• (modified) clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h (+1-1)

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;