[Openmp-commits] [PATCH] D134947: [analyzer] Fix the liveness of Symbols for values in regions referred by LazyCompoundVal

[Tomasz Kamiński via Phabricator via Openmp-commits]

tomasz-kaminski-sonarsource marked an inline comment as done.
tomasz-kaminski-sonarsource added inline comments.


================
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());
----------------
steakhal wrote:
> NoQ wrote:
> > tomasz-kaminski-sonarsource wrote:
> > > martong wrote:
> > > > Just out of curiosity, do you have plans to tackle this todo sometime?
> > > We do not plan to takle it in near future.
> > Could you add a negative/FIXME test for it?
> > 
> > At a glance I suspect that this TODO is significantly less important for `isLiveRegion()` than it is for your new function, so I encourage you to explore the possibility of dropping `getBaseRegion()`, even if just a little bit and doesn't have to be in this patch.
> > 
> > If a smaller subregion is truly live, any value inside of the base region can theoretically be accessed through safe pointer arithmetic. It's very difficult to prove that it can't be accessed anymore. Every pointer escape will be a potential access.
> > 
> > In your case, however, if the superregion is neither live nor lazily copied, the information outside of the lazily copied subregion is truly lost, there's literally nothing the program can do to recover it.
> > Could you add a negative/FIXME test for it?
> > 
> > At a glance I suspect that this TODO is significantly less important for `isLiveRegion()` than it is for your new function, so I encourage you to explore the possibility of dropping `getBaseRegion()`, even if just a little bit and doesn't have to be in this patch.
> So far we could not come up with a test case demonstrating this case.
> Right now we don't plan to investigate this area either in the foreseeable future.
@NoQ We were banging our heads against this question, and we haven't been able to create or find any example when using base region would cause a problem. Moreover, we concluded that constructing an example, where the current approach would differ in reported issues, is probably impossible.

To illustrate this I’ll refer back to the example in the summary of this patch.
The side effect of our change is that for symbol `reg_<d.z>`, representing the falsely readable location, didn’t have any binding. `isLive(reg_<d.z>)` will return `true`. However, to observe the effect, either:
a) the code needs to be able to read the `reg_<d.z>`
b) we check if we should preserve constraint on the `reg_<d.z>`

If we consider option `(a)`, that means that the code still has a reference/pointer to objects `d`, `d.z`, or to the copy of either of them. In these situations, the presence of such pointer/copy should make `reg_<d.z>` live - regardless of the existence of `lazyCompoundVal` to subregions of `d`, so `isLive(reg_<d.z>)` would return `true` anyway.

In the case of `(b)`, we will preserve all constraints that refer to `reg_<d.z>`. If `reg_<d.z>` would be reachable/accessible, similar reasoning as for option `(a)` would conclude that it must be live anyway. In contrast, when the program can no longer reach/access the value of `d.z`, the presence of this constraint cannot impact the result of the analysis, hence it would do no harm.

Given the tradeoff between additional dormant constraints and the complexity (and cost) of additional checking in `SymbolReaper`, we believe that using the base region is the right choice, and we should simply replace `TODO` with an appropriate explanation.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134947