[analyzer][NFC] Improve documentation of invalidateRegion methods

[NagyDonat]

... within the classes StoreManager and ProgramState and describe the connection between the two methods.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Donát Nagy (NagyDonat)

Changes

... within the classes StoreManager and ProgramState and describe the connection between the two methods.

---

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

2 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h (+14-11)
• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (+9-2)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
index 51d76dc257ee94..c02efdbd794eb0 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -304,24 +304,27 @@ class ProgramState : public llvm::FoldingSetNode {
 
   [[nodiscard]] ProgramStateRef killBinding(Loc LV) const;
 
-  /// Returns the state with bindings for the given regions
-  ///  cleared from the store.
+  /// Returns the state with bindings for the given regions cleared from the
+  /// store. If \p Call is non-null, also invalidates global regions (but if
+  /// \p Call is from a system header, then this is limited to globals declared
+  /// in system headers).
   ///
-  /// Optionally invalidates global regions as well.
+  /// This calls the lower-level method \c StoreManager::invalidateRegions to
+  /// do the actual invalidation, then calls the checker callbacks which should
+  /// be triggered by this event.
   ///
   /// \param Regions the set of regions to be invalidated.
   /// \param E the expression that caused the invalidation.
   /// \param BlockCount The number of times the current basic block has been
-  //         visited.
-  /// \param CausesPointerEscape the flag is set to true when
-  ///        the invalidation entails escape of a symbol (representing a
-  ///        pointer). For example, due to it being passed as an argument in a
-  ///        call.
+  ///        visited.
+  /// \param CausesPointerEscape the flag is set to true when the invalidation
+  ///        entails escape of a symbol (representing a pointer). For example,
+  ///        due to it being passed as an argument in a call.
   /// \param IS the set of invalidated symbols.
   /// \param Call if non-null, the invalidated regions represent parameters to
   ///        the call and should be considered directly invalidated.
-  /// \param ITraits information about special handling for a particular
-  ///        region/symbol.
+  /// \param ITraits information about special handling for particular regions
+  ///        or symbols.
   [[nodiscard]] ProgramStateRef
   invalidateRegions(ArrayRef<const MemRegion *> Regions, const Expr *E,
                     unsigned BlockCount, const LocationContext *LCtx,
@@ -330,7 +333,7 @@ class ProgramState : public llvm::FoldingSetNode {
                     RegionAndSymbolInvalidationTraits *ITraits = nullptr) const;
 
   [[nodiscard]] ProgramStateRef
-  invalidateRegions(ArrayRef<SVal> Regions, const Expr *E, unsigned BlockCount,
+  invalidateRegions(ArrayRef<SVal> Values, const Expr *E, unsigned BlockCount,
                     const LocationContext *LCtx, bool CausesPointerEscape,
                     InvalidatedSymbols *IS = nullptr,
                     const CallEvent *Call = nullptr,
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
index ef23b160a3c032..e08d5e104e9c0a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -205,8 +205,15 @@ class StoreManager {
   /// invalidateRegions - Clears out the specified regions from the store,
   ///  marking their values as unknown. Depending on the store, this may also
   ///  invalidate additional regions that may have changed based on accessing
-  ///  the given regions. Optionally, invalidates non-static globals as well.
-  /// \param[in] store The initial store
+  ///  the given regions. If \p Call is non-null, then this also invalidates
+  ///  non-static globals (but if \p Call is from a system header, then this is
+  ///  limited to globals declared in system headers).
+  ///
+  /// Instead of calling this method directly, you should probably use
+  /// \c ProgramState::invalidateRegions, which calls this and then ensures that
+  /// the relevant checker callbacks are triggered.
+  ///
+  /// \param[in] store The initial store.
   /// \param[in] Values The values to invalidate.
   /// \param[in] E The current statement being evaluated. Used to conjure
   ///   symbols to mark the values of invalidated regions.

[steakhal]

LGTM.
Do you plan to apply more refactors to invalidation and Store?
I'm asking because currently we experiment with a complete Store rewrite - touching there invalidation as well.

[NagyDonat]

Do you plan to apply more refactors to invalidation and Store?

These were opportunistic unplanned refactors, I was just familiarizing myself with the invalidation logic (because I'll need to use it in loop widening), and I quickly fixed some minor issues that I spotted. Now that I know that you're also editing this code, I'll avoid low priority changes like these.

However, I'll probably need one change to make more features of the invalidation logic accessible without a CallEvent. (For example, currently the CallEvent is passed down even to RegionStoreManager::invalidateRegions where it's only used to calculate an enum GlobalsFilterKind value; while I'd like to have a variant of ProgramState::invalidateRegions that directly takes a GlobalFiltersKind instead of a CallEvent.)

I'm not planning any functional/algorithmic changes in the invalidation logic, and I think and hope that I won't need to touch other parts of the State.

What can I do to help you avoid merge conflicts?