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

[Balázs Benics via Phabricator via Openmp-commits]

steakhal added a comment.

First of all, thanks for the feedback!

In D134947#3830995 <https://reviews.llvm.org/D134947#3830995>, @xazax.hun wrote:

> If we end up going with this approach, I wonder if it would be a great time to update some of the docs here: https://clang.llvm.org/docs/analyzer/developer-docs/RegionStore.html
> Usually, we are not doing a great job keeping these documentations up to date. I think the logic to determine which symbols and regions are live and how that logic interacts with the different types of memory regions might be important enough to have some documentation on it.

Yes, I'll post a patch addressing this. Thanks for noting.

---

In D134947#3832130 <https://reviews.llvm.org/D134947#3832130>, @NoQ wrote:

> Wow thanks!!
>
> Yeah this matches my understanding of the problem. I still encourage you to test it on real-world code before committing, and carefully investigate every change in diagnostics, because symbol reaper is very convoluted and filled with insane cornercases.

That's true. We did a careful investigation and the numbers are promising even at large scale.
The upside is that even if it broke something, it does not have a significant impact. The downside is that we wished for greater improvement/impact by fixing this.

> [...] carefully investigate every change in diagnostics, [...]

I investigated multiple cases, out of which I believe all of them were intentionally affected, hence improved.
Note that however, I did not investigate all the changes but only a handful of a (representative) set due to the nature of collecting, minimizing, and understanding the reports is really time-consuming.

I'd like to proceed with this patch as-is. And possibly land further incremental step(s) on top of this, such as D135136 <https://reviews.llvm.org/D135136>.
Other than D135136 <https://reviews.llvm.org/D135136> though, we don't plan to push this area any further for the time being.



================
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());
----------------
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.


================
Comment at: clang/test/Analysis/trivial-copy-struct.cpp:98
+      // w->head.next and n->next are equal
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+    }
----------------
tomasz-kaminski-sonarsource wrote:
> NoQ wrote:
> > martong wrote:
> > > 
> > Do you know what's causing this to not work? Is this a regression or just never worked?
> This example never worked. We have an in-progress fix, that we are testing now.
Fixed by D135136.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134947