Automerge: [NFCI][analyzer] Make CallEvent::getState protected (#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
6420da6 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 and do not change during the
evaluation of 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.

---------

Co-authored-by: Artem Dergachev <[email protected]>

Author: 2 people

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

@@ -211,6 +211,16 @@ class CallEvent {
   getExtraInvalidatedValues(ValueList &Values,
                             RegionAndSymbolInvalidationTraits *ETraits) const {}
 
+  /// 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 (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:
   CallEvent &operator=(const CallEvent &) = delete;
   virtual ~CallEvent() = default;
@@ -231,8 +241,11 @@ class CallEvent {
   }
   void setForeign(bool B) const { Foreign = B; }
 
-  /// The state in which the call is being evaluated.
-  const ProgramStateRef &getState() const { return State; }
+  /// NOTE: There are plans for refactoring that would eliminate this method.
+  /// Prefer to use CheckerContext::getASTContext if possible!
+  const ASTContext &getASTContext() const {
+    return getState()->getStateManager().getContext();
+  }
 
   /// The context in which the call is being evaluated.
   const LocationContext *getLocationContext() const { return LCtx; }

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

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;
 

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;
 
@@ -214,8 +214,8 @@ void ObjCSuperDeallocChecker::diagnoseCallArguments(const CallEvent &CE,
   }
 }
 
-void
-ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(ASTContext &Ctx) const {
+void 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.getState()->getStateManager().getContext();
+  const ASTContext &Ctx = M.getASTContext();
   initIdentifierInfoAndSelectors(Ctx);
 
   return M.getSelector() == SELdealloc;

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())

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;