[clang][dataflow] Only skip ExprWithCleanups when visiting terminators

`IgnoreParenImpCasts` will remove implicit casts to bool
(e.g. `PointerToBoolean`), such that the resulting expression may not
be of the `bool` type. The `cast_or_null<BoolValue>` in
`extendFlowCondition` will then trigger an assert, as the pointer
expression will not have a `BoolValue`.

Instead, we only skip `ExprWithCleanups` and `ParenExpr` nodes, as the
CFG does not emit them.

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

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

@@ -172,6 +172,10 @@ public:
   /// 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.
@@ -191,11 +195,16 @@ public:
   /// 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
@@ -226,6 +235,12 @@ public:
 
   /// 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

@@ -35,9 +35,19 @@ public:
 ///
 /// Requirements:
 ///
-///  The type of `S` must not be `ParenExpr`.
+///  `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/DataflowEnvironment.cpp

@@ -15,6 +15,7 @@
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -342,6 +343,7 @@ 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.
@@ -364,12 +366,14 @@ 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;
 }
 
 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());
   return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -33,7 +33,7 @@
 namespace clang {
 namespace dataflow {
 
-static const Expr *skipExprWithCleanups(const Expr *E) {
+const Expr *ignoreExprWithCleanups(const Expr *E) {
   if (auto *C = dyn_cast_or_null<ExprWithCleanups>(E))
     return C->getSubExpr();
   return E;
@@ -155,9 +155,7 @@ public:
       return;
     }
 
-    // The CFG does not contain `ParenExpr` as top-level statements in basic
-    // blocks, however sub-expressions can still be of that type.
-    InitExpr = skipExprWithCleanups(D.getInit()->IgnoreParens());
+    InitExpr = ignoreExprWithCleanups(D.getInit());
     assert(InitExpr != nullptr);
 
     if (D.getType()->isReferenceType()) {
@@ -190,10 +188,7 @@ public:
   }
 
   void VisitImplicitCastExpr(const ImplicitCastExpr *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->getSubExpr() != nullptr);
-    const Expr *SubExpr = S->getSubExpr()->IgnoreParens();
+    const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     switch (S->getCastKind()) {
@@ -252,10 +247,7 @@ public:
   }
 
   void VisitUnaryOperator(const UnaryOperator *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->getSubExpr() != nullptr);
-    const Expr *SubExpr = S->getSubExpr()->IgnoreParens();
+    const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     switch (S->getOpcode()) {
@@ -444,9 +436,6 @@ public:
 
   void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *S) {
     if (S->getCastKind() == CK_ConstructorConversion) {
-      // The CFG does not contain `ParenExpr` as top-level statements in basic
-      // blocks, however sub-expressions can still be of that type.
-      assert(S->getSubExpr() != nullptr);
       const Expr *SubExpr = S->getSubExpr();
       assert(SubExpr != nullptr);
 
@@ -604,7 +593,7 @@ private:
 };
 
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
-  assert(!isa<ParenExpr>(&S));
+  assert(!(isa<ParenExpr, ExprWithCleanups>(&S)));
   TransferVisitor(StmtToEnv, Env).Visit(&S);
 }
 

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

@@ -77,26 +77,26 @@ public:
       : StmtToEnv(StmtToEnv), Env(Env), BlockSuccIdx(BlockSuccIdx) {}
 
   void VisitIfStmt(const IfStmt *S) {
-    auto *Cond = S->getCond()->IgnoreParenImpCasts();
+    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }
 
   void VisitWhileStmt(const WhileStmt *S) {
-    auto *Cond = S->getCond()->IgnoreParenImpCasts();
+    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {
     assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
-    auto *LHS = S->getLHS()->IgnoreParenImpCasts();
+    auto *LHS = ignoreExprWithCleanups(S->getLHS())->IgnoreParens();
     assert(LHS != nullptr);
     extendFlowCondition(*LHS);
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
-    auto *Cond = S->getCond()->IgnoreParenImpCasts();
+    auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

@@ -1152,4 +1152,38 @@ TEST_F(FlowConditionTest, OpaqueFlowConditionInsideBranchMergesToOpaqueBool) {
       });
 }
 
+TEST_F(FlowConditionTest, PointerToBoolImplicitCast) {
+  std::string Code = R"(
+    void target(int *Ptr) {
+      bool Foo = false;
+      if (Ptr) {
+        Foo = true;
+        /*[[p1]]*/
+      }
+
+      (void)0;
+      /*[[p2]]*/
+    }
+  )";
+  runDataflow(
+      Code, [](llvm::ArrayRef<
+                   std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                   Results,
+               ASTContext &ASTCtx) {
+        ASSERT_THAT(Results, ElementsAre(Pair("p2", _), Pair("p1", _)));
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+        ASSERT_THAT(FooDecl, NotNull());
+
+        const Environment &Env1 = Results[1].second.Env;
+        auto &FooVal1 =
+            *cast<BoolValue>(Env1.getValue(*FooDecl, SkipPast::Reference));
+        EXPECT_TRUE(Env1.flowConditionImplies(FooVal1));
+
+        const Environment &Env2 = Results[0].second.Env;
+        auto &FooVal2 =
+            *cast<BoolValue>(Env2.getValue(*FooDecl, SkipPast::Reference));
+        EXPECT_FALSE(Env2.flowConditionImplies(FooVal2));
+      });
+}
+
 } // namespace