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