[PATCH] D134947: [analyzer] Fix liveness of Symbols for values in regions reffered by LazyCompoundVal

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I like the approach of this patch and I think this is somewhat aligned with @NoQ's ideas about

> a list of explicitly-live compound values

and

> "weak region roots" that aren't necessarily live themselves but anything derived from them ... is live

Coupled with the new tests for regression cases in D134941 <https://reviews.llvm.org/D134941>, I think this is really good.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:586
+  RegionSetTy LiveRegionRoots;
+  RegionSetTy LazilyCopiedRegionRoots;
 
----------------
Could you please incorporate the definition of //lazily copied locations (regions)// from the summary to here as a comment?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:653
+  bool isLazilyCopiedRegion(const MemRegion *region) const;
+  bool isReadableRegion(const MemRegion *region);
+
----------------
Could you please incorporate the definition of //readable locations (regions)// from the summary to here as a comment?


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2841-2842
           V.getAs<nonloc::LazyCompoundVal>()) {
+    // TODO: Make regions referred to by `lazyCompoundVals` that are bound to
+    // subregions of the `LCS.getRegion()` also lazily copied.
+    if (const MemRegion *R = LCS->getRegion())
----------------
Just a nit, I wonder if you might have a test case for this (which should fail for now).


================
Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:461
+bool SymbolReaper::isLazilyCopiedRegion(const MemRegion *MR) const {
+  // TODO: See comment in isLiveRegion.
+  return LazilyCopiedRegionRoots.count(MR->getBaseRegion());
----------------
Just out of curiosity, do you have plans to tackle this todo sometime?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134947/new/

https://reviews.llvm.org/D134947