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

tomasz-kaminski-sonarsource marked 2 inline comments as done. tomasz-kaminski-sonarsource added a comment. Where should I add As a result of our internal test on around ~170 projects (~20 Widnows, ~150 Linux) that are compromised of several hundreds of millions of lines of code, the impact on

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

steakhal added inline comments. Comment at: clang/test/Analysis/trivial-copy-struct.cpp:58 + clang_analyzer_warnIfReached(); // no-warning: Dead code. +}; + Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

steakhal added a comment. First of all, thanks for the feedback! In 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: >

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

tomasz-kaminski-sonarsource added inline comments. Comment at: clang/test/Analysis/trivial-copy-struct.cpp:98 + // w->head.next and n->next are equal + clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} +} NoQ wrote: > martong wrote:

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

NoQ added a comment. 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.

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

xazax.hun accepted this revision. xazax.hun added a comment. I also like the approach, but wait for @NoQ, he has the most experience in this area :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134947/new/ https://reviews.llvm.org/D134947

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

xazax.hun added a comment. 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

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

tomasz-kaminski-sonarsource updated this revision to Diff 464684. tomasz-kaminski-sonarsource added a comment. Applied suggested comment updates. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134947/new/ https://reviews.llvm.org/D134947 Files:

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

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks for the updates. I am okay with it now. LGTM. But please wait for NoQ's approval. So, this is a gentle ping for you @NoQ :) Comment at:

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

tomasz-kaminski-sonarsource updated this revision to Diff 464678. tomasz-kaminski-sonarsource added a comment. Fighting with arcanist. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134947/new/ https://reviews.llvm.org/D134947 Files:

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

tomasz-kaminski-sonarsource updated this revision to Diff 464677. tomasz-kaminski-sonarsource added a comment. Herald added subscribers: openmp-commits, zero9178, bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, mravishankar, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1,

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

tomasz-kaminski-sonarsource added a comment. Applied all review suggestions. Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:461 +bool SymbolReaper::isLazilyCopiedRegion(const MemRegion *MR) const { + // TODO: See comment in isLiveRegion. + return

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

tomasz-kaminski-sonarsource updated this revision to Diff 464673. tomasz-kaminski-sonarsource marked 3 inline comments as done. tomasz-kaminski-sonarsource added a comment. Applied review suggestions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

tomasz-kaminski-sonarsource updated this revision to Diff 464659. tomasz-kaminski-sonarsource added a comment. Included additional tests that corresponds to TODO. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134947/new/

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

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

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

tomasz-kaminski-sonarsource created this revision. Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All.