[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

78

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

78

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

78

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

78

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

78

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

78

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

230 ↗

(On Diff #426860)

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 ↗

(On Diff #426860)

For consistency with the other comment.

41 ↗

(On Diff #426860)

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

41–42 ↗

(On Diff #426860)

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

429–430 ↗

(On Diff #426860)

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

607–608 ↗

(On Diff #426860)

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

78

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

230 ↗

(On Diff #426860)

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

429–430 ↗

(On Diff #426860)

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/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

 //===----------------------------------------------------------------------===//
 
 #include <memory>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Analysis/Analyses/PostOrderCFGView.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/DataflowWorklist.h"
 #include "clang/Analysis/FlowSensitive/Transfer.h"
 #include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
static int blockIndexInPredecessor(const CFGBlock &Pred,
                                    const CFGBlock &Block) {
   auto BlockPos = llvm::find_if(
       Pred.succs(), [&Block](const CFGBlock::AdjacentBlock &Succ) {
         return Succ && Succ->getBlockID() == Block.getBlockID();
       });
   return BlockPos - Pred.succ_begin();
 }
 
+/// Skips past any parentheses and implicit casts for `E`, except if they would
+/// perform a type conversion to bool.
+///
+/// FIXME: Its hard to tell if this is a principled solution or workaround to an
+/// issue. Re-evaluate the necessity for this function after we model pointers
+/// and integers in the framework.
+static const Expr *ignoreParenImpCastsExceptToBool(const Expr *E) {
+  const Expr *LastE = nullptr;
+  while (E != LastE) {
+    LastE = E;
+    E = IgnoreParensSingleStep(const_cast<Expr *>(E));
+
+    bool WasBool = E->getType()->isBooleanType();
+    auto *Unwrapped = IgnoreImplicitCastsExtraSingleStep(const_cast<Expr *>(E));
+    if (WasBool && !Unwrapped->getType()->isBooleanType())
+      return E;
+
+    E = Unwrapped;
+  }
+  return E;
+}
+
 /// 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 = ignoreParenImpCastsExceptToBool(S->getCond());
     assert(Cond != nullptr);
     extendFlowCondition(*Cond);
   }
 
   void VisitWhileStmt(const WhileStmt *S) {
-    auto *Cond = S->getCond()->IgnoreParenImpCasts();
+    auto *Cond = ignoreParenImpCastsExceptToBool(S->getCond());
     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 = ignoreParenImpCastsExceptToBool(S->getLHS());
     assert(LHS != nullptr);
     extendFlowCondition(*LHS);
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
-    auto *Cond = S->getCond()->IgnoreParenImpCasts();
+    auto *Cond = ignoreParenImpCastsExceptToBool(S->getCond());
     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));
       });
 }
 
+// FIXME: Remove this test once it provides no additional test coverage.
+TEST_F(FlowConditionTest, DoesNotAssertForImplicitCastToBool) {
+  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