[PATCH] D55566: [analyzer] LiveVariables: Fix a zombie expression problem, add testing infrastructure.

Tue Dec 11 11:42:14 PST 2018

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, szepet, rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, baloghadamsoftware.

For the first time in years, there seems to be a bug in our "live variables" analysis, which is an auxiliary data flow analysis that decides which variables and expressions are live, so that we could collect dead symbols, clean up the program state, and, ultimately, find memory leaks.

The bug shows up on the move-checker, as demonstrated on attached tests (except the do-while test, which worked fine, but i added it to make sure it stays that way). It was originally hidden by D54372 <https://reviews.llvm.org/D54372> and seems to have been another reason why the hack that was removed in D54372 <https://reviews.llvm.org/D54372> was introduced in the first place.

Consider `checkMoreLoopZombies1()` (other tests are similar). `LiveVariables` analysis reports that //expression// `e` that constitutes the true-branch of `if` is live throughout almost the whole body of the loop (!), namely, until the beginning of `if (true)` "on the next iteration" (quotes because it doesn't really make sense for live variables analysis, but kinda makes sense for us to think of it this way). There's a brief period when it doesn't live, but it quickly becomes live again. The expression, being an lvalue, evaluates to the `VarRegion` for variable `e`. This means that at least one of the two - the variable `e` itself and the expression that evaluates to the region `e` - is always live, and region `e` never becomes dead, and the moved-from state never gets cleaned up, which leads to an invalid use-after-move report on the second iteration of the loop.

Expressions don't need to be live after the first statement that contains them is evaluated. So the liveness analysis was overly conservative in this case, and it caused problems for a checker that relies on values being cleaned up perfectly.

It was essential that the expression `e` appears directly as a sub-statement of `if`, i.e. written without `{`...`}`. Otherwise the compound statement would have been marked as live instead, which is kinda fine, even if it doesn't make sense.

Because of that, when the `if`-statement is being evaluated as a terminator, the generic code that works for pretty much all statements was marking the expression as live:

  for (Stmt *Child : S->children()) {
    if (Child)
      AddLiveStmt(val.liveStmts, LV.SSetFact, Child);

This caused `e` to be incorrectly marked as live at the end of block that terminates at `if (true)`, which is propagated to the beginning of that block (similarly to how in `x ? y : z` expressions `y` and `z` need to live at operator `x ?`, so that the operator could have been evaluated), which is then propagated to the while-loop terminator.

This is fixed by specializing the generic case for these control flow operators to avoid the unnecessary propagation of non-compound-statement branch liveness.

-----

Because i wanted to add more direct tests for this stuff (as we had none), i had to introduce a new debug checker to dump statement liveness: `debug.DumpLiveStmts`. Dumps should be human-readable and testable.

These direct tests indicate that the same incorrect behavior does indeed occur in the do-while loop case, even if it doesn't have much consequences for the move-checker.

-----

I'll add more tests if i notice that this change increases the number of leaks Static Analyzer finds.

Repository:
  rC Clang

https://reviews.llvm.org/D55566

Files:
  include/clang/Analysis/Analyses/LiveVariables.h
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/Analysis/LiveVariables.cpp
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  test/Analysis/live-stmts.cpp
  test/Analysis/use-after-move.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55566.177743.patch
Type: text/x-patch
Size: 11106 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181211/601fc67a/attachment-0001.bin>