[PATCH] D124807: [clang][dataflow] Avoid assert for invalid cast to BoolValue

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua marked 4 inline comments as done.
li.zhe.hua added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78
+/// and integers in the framework.
+static const Expr *ignoreParenImpCastsExceptToBool(const Expr *E) {
+  const Expr *LastE = nullptr;
----------------
sgatev wrote:
> xazax.hun wrote:
> > li.zhe.hua wrote:
> > > sgatev wrote:
> > > > 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
> > > 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.
> > 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. 
> 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 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()` [[ https://github.com/llvm/llvm-project/blob/ed1b32791dbb4c02cd761742a63cdf1e9d644ae6/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L108 | 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` ([[ https://godbolt.org/z/fo846dcEa | 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124807/new/

https://reviews.llvm.org/D124807