[clang][dataflow] Centralize expression skipping logic

A follow-up to 62b2a47 to centralize the logic that skips expressions
that the CFG does not emit. This allows client code to avoid
sprinkling this logic everywhere.

Add redirects in the transfer function to similarly skip such
expressions by forwarding the visit to the sub-expression.

Differential Revision: https://reviews.llvm.org/D124965

Author: tJener

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

@@ -31,6 +31,19 @@
 namespace clang {
 namespace dataflow {
 
+/// Skip past nodes that the CFG does not emit. These nodes are invisible to
+/// flow-sensitive analysis, and should be ignored as they will effectively not
+/// exist.
+///
+///   * `ParenExpr` - The CFG takes the operator precedence into account, but
+///   otherwise omits the node afterwards.
+///
+///   * `ExprWithCleanups` - The CFG will generate the appropriate calls to
+///   destructors and then omit the node.
+///
+const Expr &ignoreCFGOmittedNodes(const Expr &E);
+const Stmt &ignoreCFGOmittedNodes(const Stmt &S);
+
 /// Owns objects that encompass the state of a program and stores context that
 /// is used during dataflow analysis.
 class DataflowAnalysisContext {
@@ -95,14 +108,15 @@ class DataflowAnalysisContext {
   ///
   ///  `E` must not be assigned a storage location.
   void setStorageLocation(const Expr &E, StorageLocation &Loc) {
-    assert(ExprToLoc.find(&E) == ExprToLoc.end());
-    ExprToLoc[&E] = &Loc;
+    const Expr &CanonE = ignoreCFGOmittedNodes(E);
+    assert(ExprToLoc.find(&CanonE) == ExprToLoc.end());
+    ExprToLoc[&CanonE] = &Loc;
   }
 
   /// Returns the storage location assigned to `E` or null if `E` has no
   /// assigned storage location.
   StorageLocation *getStorageLocation(const Expr &E) const {
-    auto It = ExprToLoc.find(&E);
+    auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
     return It == ExprToLoc.end() ? nullptr : It->second;
   }
 

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

@@ -172,10 +172,6 @@ class Environment {
   /// Creates a storage location for `E`. Does not assign the returned storage
   /// location to `E` in the environment. Does not assign a value to the
   /// returned storage location in the environment.
-  ///
-  /// Requirements:
-  ///
-  ///  `E` must not be a `ExprWithCleanups`.
   StorageLocation &createStorageLocation(const Expr &E);
 
   /// Assigns `Loc` as the storage location of `D` in the environment.
@@ -195,16 +191,11 @@ class Environment {
   /// Requirements:
   ///
   ///  `E` must not be assigned a storage location in the environment.
-  ///  `E` must not be a `ExprWithCleanups`.
   void setStorageLocation(const Expr &E, StorageLocation &Loc);
 
   /// Returns the storage location assigned to `E` in the environment, applying
   /// the `SP` policy for skipping past indirections, or null if `E` isn't
   /// assigned a storage location in the environment.
-  ///
-  /// Requirements:
-  ///
-  ///  `E` must not be a `ExprWithCleanups`.
   StorageLocation *getStorageLocation(const Expr &E, SkipPast SP) const;
 
   /// Returns the storage location assigned to the `this` pointee in the
@@ -235,12 +226,6 @@ class Environment {
 
   /// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E`
   /// is assigned a storage location in the environment, otherwise returns null.
-  ///
-  /// Requirements:
-  ///
-  ///  `E` must not be a `ExprWithCleanups`.
-  ///
-  /// FIXME: `Environment` should ignore any `ExprWithCleanups` it sees.
   Value *getValue(const Expr &E, SkipPast SP) const;
 
   /// Transfers ownership of `Loc` to the analysis context and returns a

clang/include/clang/Analysis/FlowSensitive/Transfer.h

@@ -38,16 +38,6 @@ class StmtToEnvMap {
 ///  `S` must not be `ParenExpr` or `ExprWithCleanups`.
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);
 
-/// Skip past a `ExprWithCleanups` which might surround `E`. Returns null if `E`
-/// is null.
-///
-/// The CFG omits `ExprWithCleanups` nodes (as it does with `ParenExpr`), and so
-/// the transfer function doesn't accept them as valid input. Manual traversal
-/// of the AST should skip and unwrap any `ExprWithCleanups` it might expect to
-/// see. They are safe to skip, as the CFG will emit calls to destructors as
-/// appropriate.
-const Expr *ignoreExprWithCleanups(const Expr *E);
-
 } // namespace dataflow
 } // namespace clang
 

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include <cassert>
 #include <memory>
@@ -155,3 +156,22 @@ void DataflowAnalysisContext::addTransitiveFlowConditionConstraints(
 
 } // namespace dataflow
 } // namespace clang
+
+using namespace clang;
+
+const Expr &clang::dataflow::ignoreCFGOmittedNodes(const Expr &E) {
+  const Expr *Current = &E;
+  if (auto *EWC = dyn_cast<ExprWithCleanups>(Current)) {
+    Current = EWC->getSubExpr();
+    assert(Current != nullptr);
+  }
+  Current = Current->IgnoreParens();
+  assert(Current != nullptr);
+  return *Current;
+}
+
+const Stmt &clang::dataflow::ignoreCFGOmittedNodes(const Stmt &S) {
+  if (auto *E = dyn_cast<Expr>(&S))
+    return ignoreCFGOmittedNodes(*E);
+  return S;
+}

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

@@ -343,7 +343,6 @@ StorageLocation &Environment::createStorageLocation(const VarDecl &D) {
 }
 
 StorageLocation &Environment::createStorageLocation(const Expr &E) {
-  assert(!isa<ExprWithCleanups>(&E));
   // Evaluated expressions are always assigned the same storage locations to
   // ensure that the environment stabilizes across loop iterations. Storage
   // locations for evaluated expressions are stored in the analysis context.
@@ -366,16 +365,15 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
 }
 
 void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
-  assert(!isa<ExprWithCleanups>(&E));
-  assert(ExprToLoc.find(&E) == ExprToLoc.end());
-  ExprToLoc[&E] = &Loc;
+  const Expr &CanonE = ignoreCFGOmittedNodes(E);
+  assert(ExprToLoc.find(&CanonE) == ExprToLoc.end());
+  ExprToLoc[&CanonE] = &Loc;
 }
 
 StorageLocation *Environment::getStorageLocation(const Expr &E,
                                                  SkipPast SP) const {
-  assert(!isa<ExprWithCleanups>(&E));
   // FIXME: Add a test with parens.
-  auto It = ExprToLoc.find(E.IgnoreParens());
+  auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
   return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
 }
 

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -33,27 +33,12 @@
 namespace clang {
 namespace dataflow {
 
-const Expr *ignoreExprWithCleanups(const Expr *E) {
-  if (auto *C = dyn_cast_or_null<ExprWithCleanups>(E))
-    return C->getSubExpr();
-  return E;
-}
-
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
                                           Environment &Env) {
-  // Equality of booleans involves implicit integral casts. Ignore these casts
-  // for now and focus on the values associated with the wrapped expressions.
-  // FIXME: Consider changing this once the framework offers better support for
-  // integral casts.
-  const Expr *LHSNorm = LHS.IgnoreCasts();
-  const Expr *RHSNorm = RHS.IgnoreCasts();
-  assert(LHSNorm != nullptr);
-  assert(RHSNorm != nullptr);
-
-  if (auto *LHSValue = dyn_cast_or_null<BoolValue>(
-          Env.getValue(*LHSNorm, SkipPast::Reference)))
-    if (auto *RHSValue = dyn_cast_or_null<BoolValue>(
-            Env.getValue(*RHSNorm, SkipPast::Reference)))
+  if (auto *LHSValue =
+          dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
+    if (auto *RHSValue =
+            dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
       return Env.makeIff(*LHSValue, *RHSValue);
 
   return Env.makeAtomicBoolValue();
@@ -65,14 +50,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       : StmtToEnv(StmtToEnv), Env(Env) {}
 
   void VisitBinaryOperator(const BinaryOperator *S) {
-    // The CFG does not contain `ParenExpr` as top-level statements in basic
-    // blocks, however sub-expressions can still be of that type.
-    assert(S->getLHS() != nullptr);
-    const Expr *LHS = S->getLHS()->IgnoreParens();
+    const Expr *LHS = S->getLHS();
     assert(LHS != nullptr);
 
-    assert(S->getRHS() != nullptr);
-    const Expr *RHS = S->getRHS()->IgnoreParens();
+    const Expr *RHS = S->getRHS();
     assert(RHS != nullptr);
 
     switch (S->getOpcode()) {
@@ -155,7 +136,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       return;
     }
 
-    InitExpr = ignoreExprWithCleanups(D.getInit());
+    InitExpr = D.getInit();
     assert(InitExpr != nullptr);
 
     if (D.getType()->isReferenceType()) {
@@ -476,8 +457,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       assert(S->getArg(0) != nullptr);
       // `__builtin_expect` returns by-value, so strip away any potential
       // references in the argument.
-      auto *ArgLoc = Env.getStorageLocation(
-          *S->getArg(0)->IgnoreParenImpCasts(), SkipPast::Reference);
+      auto *ArgLoc = Env.getStorageLocation(*S->getArg(0), SkipPast::Reference);
       if (ArgLoc == nullptr)
         return;
       Env.setStorageLocation(*S, *ArgLoc);
@@ -562,6 +542,24 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     Env.setValue(Loc, Env.getBoolLiteralValue(S->getValue()));
   }
 
+  void VisitParenExpr(const ParenExpr *S) {
+    // The CFG does not contain `ParenExpr` as top-level statements in basic
+    // blocks, however manual traversal to sub-expressions may encounter them.
+    // Redirect to the sub-expression.
+    auto *SubExpr = S->getSubExpr();
+    assert(SubExpr != nullptr);
+    Visit(SubExpr);
+  }
+
+  void VisitExprWithCleanups(const ExprWithCleanups *S) {
+    // The CFG does not contain `ExprWithCleanups` as top-level statements in
+    // basic blocks, however manual traversal to sub-expressions may encounter
+    // them. Redirect to the sub-expression.
+    auto *SubExpr = S->getSubExpr();
+    assert(SubExpr != nullptr);
+    Visit(SubExpr);
+  }
+
 private:
   BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
     // `SubExpr` and its parent logic operator might be part of different basic
@@ -593,7 +591,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 };
 
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
-  assert(!(isa<ParenExpr, ExprWithCleanups>(&S)));
   TransferVisitor(StmtToEnv, Env).Visit(&S);
 }
 

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

@@ -46,7 +46,7 @@ class StmtToEnvMapImpl : public StmtToEnvMap {
       : CFCtx(CFCtx), BlockToState(BlockToState) {}
 
   const Environment *getEnvironment(const Stmt &S) const override {
-    auto BlockIT = CFCtx.getStmtToBlock().find(&S);
+    auto BlockIT = CFCtx.getStmtToBlock().find(&ignoreCFGOmittedNodes(S));
     assert(BlockIT != CFCtx.getStmtToBlock().end());
     const auto &State = BlockToState[BlockIT->getSecond()->getBlockID()];
     assert(State.hasValue());
@@ -77,26 +77,26 @@ class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> {
       : StmtToEnv(StmtToEnv), Env(Env), BlockSuccIdx(BlockSuccIdx) {}
 
   void VisitIfStmt(const IfStmt *S) {
-    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+    auto *Cond = S->getCond();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }
 
   void VisitWhileStmt(const WhileStmt *S) {
-    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+    auto *Cond = S->getCond();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {
     assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
-    auto *LHS = ignoreExprWithCleanups(S->getLHS())->IgnoreParens();
+    auto *LHS = S->getLHS();
     assert(LHS != nullptr);
     extendFlowCondition(*LHS);
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
-    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+    auto *Cond = S->getCond();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }