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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
