[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

This revision was automatically updated to reflect the committed changes. Closed by commit rGdd1d5488e47d: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing… (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun accepted this revision. xazax.hun added a comment. So now all of the dependencies are landed and this should be ready for its prime time, right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598 _

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus updated this revision to Diff 291231. Szelethus added a comment. Rename the live statements checker to live expressions checker. The test file added in a revert commit changed rather heavily, but it makes sense that these entries are removed IMO. Unless anyone objects, I'll intend to l

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. In D82598#2172416 , @NoQ wrote: > I still wonder what made this statement live in my example. There must have > been some non-trivial liveness analysis going on that caused a statement to > be live; probably something to do wit

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ added a comment. I still wonder what made this statement live in my example. There must have been some non-trivial liveness analysis going on that caused a statement to be live; probably something to do with the C++ destructor elements. In D82598#2172371

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. Wow, I never realized I accidentally landed that assert (D82122#2172360 ), but I guess its great to have that covered. Would you prefer to have that reverted as I'm looking to fix this for good? Repository: rG LLVM Github M

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. Thank you so much, you really went out of your way to dig that out! I'll try to patch this somehow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ added a comment. Ok, here's the crashing example with `ObjCForCollectionStmt`. It should be saved as an `.mm` file and it crashes under pure `--analyze`. @interface Item // ... @end @interface Collection // ... @end typedef void (^Blk)(); struct RAII { Blk blk;

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2164363 , @Szelethus wrote: > Actually, what I said initially is true: > > > [...] the only non-expression statements it **queried** are > > ObjCForCollectionStmts [...] Btw, sorry. I somehow misunderstood the snippe

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. Ouch. Let me know how severe this is, because this is a big milestone in my project. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598 ___

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ added a comment. In D82598#2164363 , @Szelethus wrote: > > Hmm, interesting. I don't really understand why do we need to keep that > > block live, as we definitely won't use any of the value it provides (since > > it does not provide a value at all).

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:317-320 for (Stmt *Child : S->children()) { -if (Child) - AddLiveStmt(val.liveStmts, LV.SSetFact, Child); +if (const auto *E = dyn_cast_or

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:317-320 for (Stmt *Child : S->children()) { -if (Child) - AddLiveStmt(val.liveStmts, LV.SSetFact, Child); +if (const auto *E = dyn_cast_or_null(Child)) + AddLiveExpr(val.liveExp

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus marked an inline comment as done. Szelethus added a comment. In D82598#2162496 , @xazax.hun wrote: > In D82598#2162239 , @Szelethus wrote: > > > I chased my own tail for weeks before realizing that there i

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2162239 , @Szelethus wrote: > I chased my own tail for weeks before realizing that there is indeed another > instance when a live **statement** is stored, other then > `ObjCForCollectionStmt`... > > void clang_analy

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. I chased my own tail for weeks before realizing that there is indeed another instance when a live **statement** is stored, other then `ObjCForCollectionStmt`... void clang_analyzer_eval(bool); void test_lambda_refcapture() { int a = 6; [&](int &a) { a =

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ added a comment. In D82598#2119545 , @xazax.hun wrote: > > Given that we'd also barely ever notice that we forgot one of those, i'm > > very much in favor of having liveness analysis instead, that would > > declaratively describe which expressions ar

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. Note to self: rename `debug.DumpLiveStmts` to `debug.DumpLiveExprs`. Thanks for the review btw! I don't immediately have something to add to your discussion though :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/n

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun accepted this revision. xazax.hun added a comment. @szelethus The patch looks good to me and I find it a welcome change that should make things easier to understand and maybe even a bit more efficient, I hope this won't break ObjC :) My discussion with Artem is orthogonal to this chang

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ added a comment. In D82598#2116373 , @xazax.hun wrote: > > I suspect that it would be pretty bad if you, say, kill the condition of > > the `if`-statement before picking the branch. Or kill the initializer in > > the `DeclStmt` before putting it into

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2115656 , @NoQ wrote: > > We could just kill all subexpr at the end of the full expression > > I suspect that it would be pretty bad if you, say, kill the condition of the > `if`-statement before picking the branch. Or

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. > We could just kill all subexpr at the end of the full expression I suspect that it would be pretty bad if you, say, kill the condition of the `if`-statement before picking the branch. Or kill the

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. I always wondered if we actually need to track the liveness of exprs at all. We could just kill all subexpr at the end of the full expression without precomputing anything. But I might miss something. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus created this revision. Szelethus added reviewers: NoQ, xazax.hun, vsavchenko, martong, dcoughlin. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, balo

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

Szelethus added a comment. I didn't run this on big projects just yet, and judging from the fact that `Environment` only contained ObjetiveC examples of non-expression statements, I might need a tip on how to test on ObjC code :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION