[clang] [NFCI][analyzer] Make CallEvent::getState protected (PR #162673)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Donát Nagy (NagyDonat)

<details>
<summary>Changes</summary>

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

---
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
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;

``````````

</details>


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