[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

[Kristóf Umann via Phabricator via cfe-commits]

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

I had a bit of fun with the tags.

Liveness analysis calculates the set of live/dead values in the program. However, statements don't always have a value associated with them, expressions do. For this reason, I found it puzzling why LivenessAnalysis work with "live statements", or answers questions such as "is this statement live". After a bit of digging, I found that the only client of statement liveness is `EnvironmentManager::removeDeadBindings` (the one modified in this patch), and the only non-expression statement it queries are `ObjCForCollectionStmt`s, but no lit tests crashed on the added asserts.

This patch isn't necesserily intended to be committed -- it more of a request for comments whether it would be safe to change statement liveness to strictly expression liveness. It would be great, because my ultimate goal is to merge the implementation of UninitializedVariable and LivenessAnalaysis, and add Reaching Definitions under the same hood, but the statement liveness was very much out of place and makes the merge very awkward, if even possible.

I have a number of changes planned, but I didn't want to go too far ahead of myself, in case I missed something fundamental that justifies statement liveness as it is.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82122

Files:
  clang/lib/StaticAnalyzer/Core/Environment.cpp


Index: clang/lib/StaticAnalyzer/Core/Environment.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/Environment.cpp
+++ clang/lib/StaticAnalyzer/Core/Environment.cpp
@@ -183,12 +183,18 @@
              F.getTreeFactory());
 
   // Iterate over the block-expr bindings.
-  for (Environment::iterator I = Env.begin(), E = Env.end();
-       I != E; ++I) {
+  for (Environment::iterator I = Env.begin(), E = Env.end(); I != E; ++I) {
     const EnvironmentEntry &BlkExpr = I.getKey();
     const SVal &X = I.getData();
 
-    if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext())) {
+    const bool IsBlkExprLive =
+        SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext());
+
+    assert((isa<Expr>(BlkExpr.getStmt()) || !IsBlkExprLive) &&
+           "Only Exprs can be live, LivenessAnalysis argues about the liveness "
+           "of *values*!");
+
+    if (IsBlkExprLive) {
       // Copy the binding to the new map.
       EBMapRef = EBMapRef.add(BlkExpr, X);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D82122.271814.patch
Type: text/x-patch
Size: 1053 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200618/2db6846d/attachment.bin>