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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D82598#2164363 <https://reviews.llvm.org/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).
>
> Actually, what I said initially is true:
>
> > [...] the only non-expression statements it **queried** are ObjCForCollectionStmts [...]
>
> so I think it'd be okay to simply drop this.


Yeah, that sounds about right; you observed the current behavior to be a counterexample but found no evidence that the current behavior makes any sense.

P.S. We've found an example where `ObjCForCollectionStmt`s break (i.e., your recently added assertion gets hit), i'll reduce and think of a patch. Still shocked this wasn't covered with tests.



================
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<Expr>(Child))
+      AddLiveExpr(val.liveExprs, LV.SSetFact, E);
   }
----------------
Szelethus wrote:
> xazax.hun wrote:
> > Szelethus wrote:
> > > ..this part of the code caused the issue. Looking at the related CFG,
> > > ```lang=c++
> > > void test_lambda_refcapture()
> > >  [B2 (ENTRY)]
> > >    Succs (1): B1
> > > 
> > >  [B1]
> > >    1: 6
> > >    2: int a = 6;
> > >    3: operator()
> > >    4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(int &) const)
> > >    5: [&](int &a) {
> > >     a = 42;
> > > }
> > >    6: [B1.5]
> > >    7: [B1.6] (ImplicitCastExpr, NoOp, const class (lambda at /home/szelethus/Documents/llvm-project/clang/test/Analysis/live-stmts.cpp:183:3))
> > >    8: a
> > >    9: [B1.7]([B1.8]) (OperatorCall)
> > >   10: clang_analyzer_eval
> > >   11: [B1.10] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(_Bool))
> > >   12: a
> > >   13: [B1.12] (ImplicitCastExpr, LValueToRValue, int)
> > >   14: 42
> > >   15: [B1.13] == [B1.14]
> > >   16: [B1.11]([B1.15])
> > >    Preds (1): B2
> > >    Succs (1): B0
> > > 
> > >  [B0 (EXIT)]
> > >    Preds (1): B1
> > > ```
> > > its clear that element 5 added the live statement, and I think that that this entire CFG just simply isn't right. Shouldn't we have a distinct element for the assignment?
> > > 
> > >  Shouldn't we have a distinct element for the assignment?
> > 
> > Strictly speaking, we have CFGs for a function. The assignment is **not** in this function, it is in the `operator()` of the class representing this lambda expression. 
> > 
> > So basically, we do have a `LambdaExpr` to represent the expression, but the body of the lambda is in a separate entity.
> Well, `debug.DumpCFG` definitely doesn't indulge me with a separate lambda CFG, so I figured this is a (rightful) optimization or compression.
> 
> My point is, this entire code snippet is a seriously error prone, best-effort heuristic. Switch casing every small little corner case might be tedious, troublesome in terms of scaling, and I for sure don't want to maintain it 'til eternity, but we have to acknowledge that this isn't a perfect solution either.
> Well, debug.DumpCFG definitely doesn't indulge me with a separate lambda CFG

Does that mean that `checkASTCodeBody` doesn't get run on lambda bodies, and neither do any of our path-insensitive checks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82598