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

[li.zhe.hua]

Summary

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.

[li.zhe.hua]

Remove else after return

[ymandel]

Thanks for the patch!

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

1155

This seems a worthwhile case to cover. What about converting it to a more useful test, similar to the other branch tests?

[li.zhe.hua]

Update test to treat the PointerToBool conversion as an opaque boolean expression, and test it as such.

[sgatev]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

I don't recall why we need to ignore implicit casts here. Can't we ignore parens and rely on the built-in transfer function, possibly adding handling of some missing casts there? https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L192

[li.zhe.hua]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

If we only ignore parens, a test in the optional checker tests begins to fail, specifically UncheckedOptionalAccessTest.ValueOrComparison. The missing "cast" is an ExprWithCleanups. I didn't know how to deal with that, so this patch just working around the assert.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

In general, I prefer to handle as much as possible with transfer functions and skip as little as possible in the AST. We might skip ExprWithCleanups nodes today, but we will need them tomorrow to properly model where certain destructors are being invoked.

[sgatev]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

We already have skipExprWithCleanups [1]. I wonder if it makes sense to replace that with a simple transfer function like the one for the CK_NoOp implicit cast. Would that solve the problem and remove the need for ignoreParenImpCastsExceptToBool? In the future we might replace that transfer function with a proper one, as Gábor suggested.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L36

[li.zhe.hua]

Handle FullExprs in the transfer function.

[li.zhe.hua]

Switch from implementing in the trasfer function to skipping ExprWithCleanups.

[li.zhe.hua]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

I sat down with @ymandel to get a better understanding of what was happening here, which was really helpful in understanding why my random flailing was making tests pass, and what I believe to be a better way forward.

So, the CFG does not emit ParenExpr nodes. As such, the transfer function never sees them, and so when we do any kind of manual traversal (e.g. to look at a sub-expression), we use ignoreParens() because they effectively don't exist. The same thing happens with ExprWithCleanups. The CFG doesn't appear to emit them explicitly, and so we should similarly avoid them when manually traversing the AST.

Note: Their position in the AST is slightly more constrained, likely as the immediate children of *Stmt nodes. This means we don't need to sprinkle these skips as widely as ignoreParens().

In terms of why adding VisitFullExpr "fixed" the failing test: extendFlowCondition() manually calls transfer on the provided Cond expression, which is a ExprWithCleanups if the caller uses ignoreParens() instead of ignoreParenImpCasts().

---

@xazax.hun:

In general, I prefer to handle as much as possible with transfer functions and skip as little as possible in the AST. We might skip ExprWithCleanups nodes today, but we will need them tomorrow to properly model where certain destructors are being invoked.

The CFG already emits calls to destructors as a result of ExprWithCleanups (godbolt), so skipping them will not cause us to miss calls to destructors.

---

@sgatev:

We already have skipExprWithCleanups [1]. I wonder if it makes sense to replace that with a simple transfer function like the one for the CK_NoOp implicit cast. Would that solve the problem and remove the need for ignoreParenImpCastsExceptToBool? In the future we might replace that transfer function with a proper one, as Gábor suggested.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L36

I believe that ParenExpr and ExprWithCleanups are categorically the same within the framework (roughly, nodes that are not emitted by the CFG) and so we should handle them in the same way. The strategy right now for ParenExpr is to skip them, so I am inclined to do so as well with ExprWithCleanups for now. (I'm not necessarily opposed to having ParenExpr and ExprWithCleanups nodes being handled in the transfer function, but there's a question of how to get them in there if the CFG isn't going to emit them.)

That would mean more usage of skipExprWithCleanups, and is reflected in this most recent revision.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

Thanks for the detailed explanation, this sounds good to me! Could you add a short summary as a comment to ignoreExprWithCleanups?

[li.zhe.hua]

Expand ignoreExprWithCleanups comment with more context.

[ymandel]

Nice!! As Gabor said, thank you for the detailed explanation (on top of the work to get this all done in a principled manner).

[sgatev]

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

239

Why add these as requirements instead of skipping past ExprWithCleanups internally, as we currently do for parens? Should we add similar requirements to createStorageLocation, setStorageLocation, and getStorageLocation? Would that result in lots of client code sprinkled with IgnoreParens and some form of ignoreExprWithCleanups?

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

38

For consistency with the other comment.

41

I find the use of "any" confusing here. Let's clarify that it skips past at most one instance of ExprWithCleanups.

41–42

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

432–433

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

224 ↗

(On Diff #426860)

Do we have a test that fails if IgnoreParens is missing? If not, let's add a test or extend one of the existing. Perhaps https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp#L1286

clang/lib/Analysis/FlowSensitive/Transfer.cpp

596–597

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

77

Thank you for the explanation! This looks good to me too.

[li.zhe.hua]

Add ExprWithCleanups requirement to createStorageLocation, setStorageLocation, and getStorageLocation.
Remove ParenExpr requirement from getValue.
Remove unnecessary calls to IgnoreParens.
Address misc. comments; apply suggestions.

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

239

Ah, I missed that we do this automatically for parens, in part because we do unnecessarily sprinkle IgnoreParens in a lot of places in Transfer.cpp.

That said, we can't avoid sprinkling them at least in some places for the time being. Fixing this greatly expands beyond the scope of fixing a cast assert firing. (That is still, aspirationally, the goal of this patch.)

Let me remove the ParenExpr requirement (that's incorrect), leave the ExprWithCleanups requirement (with FIXME), and leave the holistic cleanup for a separate patch.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

432–433

Acknowledged. Obviated by removing the ParenExpr assert.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

224 ↗

(On Diff #426860)

I missed that we ignore parens automatically in getValue. Removing this.

[li.zhe.hua]

Forgot to add a file... let's try this again...

[sgatev]

Looks great! Thank you!

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

1156

Maybe call this PointerToBoolImplicitCast and remove the FIXME?

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

public:
   /// Creates a storage location for `D`. Does not assign the returned storage
   /// location to `D` in the environment. Does not assign a value to the
   /// returned storage location in the environment.
   StorageLocation &createStorageLocation(const VarDecl &D);
 
   /// 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.
   ///
   /// Requirements:
   ///
   ///  `D` must not be assigned a storage location in the environment.
   void setStorageLocation(const ValueDecl &D, StorageLocation &Loc);
 
   /// Returns the storage location assigned to `D` in the environment, applying
   /// the `SP` policy for skipping past indirections, or null if `D` isn't
   /// assigned a storage location in the environment.
   StorageLocation *getStorageLocation(const ValueDecl &D, SkipPast SP) const;
 
   /// Assigns `Loc` as the storage location of `E` in the 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
   /// environment or null if the `this` pointee has no assigned storage location
   /// in the environment.
   StorageLocation *getThisPointeeStorageLocation() const;
 
   /// Creates a value appropriate for `Type`, if `Type` is supported, otherwise
public:
   Value *getValue(const StorageLocation &Loc) const;
 
   /// Equivalent to `getValue(getStorageLocation(D, SP), SkipPast::None)` if `D`
   /// is assigned a storage location in the environment, otherwise returns null.
   Value *getValue(const ValueDecl &D, SkipPast SP) const;
 
   /// 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
   /// reference to it.
   ///
   /// Requirements:
   ///
   ///  `Loc` must not be null.

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

public:
   /// FIXME: Ensure that the result can't be null and return a const reference.
   virtual const Environment *getEnvironment(const Stmt &S) const = 0;
 };
 
 /// Evaluates `S` and updates `Env` accordingly.
 ///
 /// 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
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TRANSFER_H

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

 //  that run over Control-Flow Graphs (CFGs) to keep track of the state of the
 //  program at given program points.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
StorageLocation &Environment::createStorageLocation(const VarDecl &D) {
   if (auto *Loc = DACtx->getStorageLocation(D))
     return *Loc;
   auto &Loc = createStorageLocation(D.getType());
   DACtx->setStorageLocation(D, Loc);
   return Loc;
 }
 
 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.
   if (auto *Loc = DACtx->getStorageLocation(E))
     return *Loc;
   auto &Loc = createStorageLocation(E.getType());
   DACtx->setStorageLocation(E, Loc);
   return Loc;
 }
 
 void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
   assert(DeclToLoc.find(&D) == DeclToLoc.end());
   DeclToLoc[&D] = &Loc;
 }
 
 StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
                                                  SkipPast SP) const {
   auto It = DeclToLoc.find(&D);
   return It == DeclToLoc.end() ? nullptr : &skip(*It->second, SP);
 }
 
 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);
 }
 
 StorageLocation *Environment::getThisPointeeStorageLocation() const {
   return DACtx->getThisPointeeStorageLocation();
 }

 Value *Environment::getValue(const ValueDecl &D, SkipPast SP) const {
   auto *Loc = getStorageLocation(D, SP);
   if (Loc == nullptr)
     return nullptr;
   return getValue(*Loc);
 }
 
 Value *Environment::getValue(const Expr &E, SkipPast SP) const {
   auto *Loc = getStorageLocation(E, SP);
   if (Loc == nullptr)
     return nullptr;
   return getValue(*Loc);
 }
 
 Value *Environment::createValue(QualType Type) {
   llvm::DenseSet<QualType> Visited;
   int CreatedValuesCount = 0;
   Value *Val = createValueUnlessSelfReferential(Type, Visited, /*Depth=*/0,

clang/lib/Analysis/FlowSensitive/Transfer.cpp

 #include "llvm/Support/Casting.h"
 #include <cassert>
 #include <memory>
 #include <tuple>
 
 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;
 }
 
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
                                           Environment &Env) {
   // Equality of booleans involves implicit integral casts. Ignore these casts
void VisitDeclStmt(const DeclStmt *S) {
     const Expr *InitExpr = D.getInit();
     if (InitExpr == nullptr) {
       // No initializer expression - associate `Loc` with a new value.
       if (Value *Val = Env.createValue(D.getType()))
         Env.setValue(Loc, *Val);
       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()) {
       // Initializing a reference variable - do not create a reference to
       // reference.
       if (auto *InitExprLoc =
               Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
         auto &Val =
void VisitDeclStmt(const DeclStmt *S) {
     // FIXME. If and when we interpret all language cases, change this to assert
     // that `InitExpr` is interpreted, rather than supplying a default value
     // (assuming we don't update the environment API to return references).
     if (Value *Val = Env.createValue(D.getType()))
       Env.setValue(Loc, *Val);
   }
 
   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()) {
     case CK_IntegralToBoolean: {
       // This cast creates a new, boolean value from the integral value. We
       // model that with a fresh value in the environment, unless it's already a
       // boolean.
       auto &Loc = Env.createStorageLocation(*S);
case CK_NoOp: {
       break;
     }
     default:
       break;
     }
   }
 
   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()) {
     case UO_Deref: {
       // Skip past a reference to handle dereference of a dependent pointer.
       const auto *SubExprVal = cast_or_null<PointerValue>(
           Env.getValue(*SubExpr, SkipPast::Reference));
       if (SubExprVal == nullptr)
if (S->getOperator() == OO_Equal) {
       // FIXME: Add a test for the value of the whole expression.
       // Assign a storage location for the whole expression.
       Env.setStorageLocation(*S, *ObjectLoc);
     }
   }
 
   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);
 
       auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
       if (SubExprLoc == nullptr)
         return;
 
       Env.setStorageLocation(*S, *SubExprLoc);
BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
     return Env.makeAtomicBoolValue();
   }
 
   const StmtToEnvMap &StmtToEnv;
   Environment &Env;
 };
 
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
-  assert(!isa<ParenExpr>(&S));
+  assert(!(isa<ParenExpr, ExprWithCleanups>(&S)));
   TransferVisitor(StmtToEnv, Env).Visit(&S);
 }
 
 } // namespace dataflow
 } // namespace clang

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

 }
 
 /// Extends the flow condition of an environment based on a terminator
 /// statement.
 class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> {
 public:
   TerminatorVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
                     int BlockSuccIdx)
       : 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);
   }
 
 private:
   void extendFlowCondition(const Expr &Cond) {
     // The terminator sub-expression might not be evaluated.
     if (Env.getValue(Cond, SkipPast::None) == nullptr)

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

runDataflow(
 
         auto &BarVal =
             *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference));
 
         EXPECT_FALSE(Env.flowConditionImplies(BarVal));
       });
 }
 
+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