[PATCH] D124807: [clang][dataflow] Only skip ExprWithCleanups when visiting terminators

Wed May 4 00:46:55 PDT 2022

sgatev added inline comments.

================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:230
+  ///
+  /// Requirements:
+  ///
----------------
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`?

================
Comment at: clang/include/clang/Analysis/FlowSensitive/Transfer.h:38
 ///
-///  The type of `S` must not be `ParenExpr`.
+///  The type of `S` must not be `ParenExpr` or `ExprWithCleanups`.
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);
----------------
For consistency with the other comment.

================
Comment at: clang/include/clang/Analysis/FlowSensitive/Transfer.h:41

+/// Skip past any `ExprWithCleanups` which might surround `E`. Returns a null
+/// pointer if `E` is null.
----------------
I find the use of "any" confusing here. Let's clarify that it skips past at most one instance of `ExprWithCleanups`.

================
Comment at: clang/include/clang/Analysis/FlowSensitive/Transfer.h:41-42

+/// Skip past any `ExprWithCleanups` which might surround `E`. Returns a null
+/// pointer if `E` is null.
+///
----------------
sgatev wrote:
> I find the use of "any" confusing here. Let's clarify that it skips past at most one instance of `ExprWithCleanups`.

================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:429-430
 Value *Environment::getValue(const Expr &E, SkipPast SP) const {
+  assert(!isa<ParenExpr>(&E));
+  assert(!isa<ExprWithCleanups>(&E));
   auto *Loc = getStorageLocation(E, SP);
----------------

================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:224
+  if (auto *OptionalVal = cast_or_null<StructValue>(State.Env.getValue(
+          *ObjectExpr->IgnoreParens(), SkipPast::ReferenceThenPointer))) {
     auto *HasValueVal = getHasValue(OptionalVal);
----------------
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

================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:607-608
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
   assert(!isa<ParenExpr>(&S));
+  assert(!isa<ExprWithCleanups>(&S));
   TransferVisitor(StmtToEnv, Env).Visit(&S);
----------------

================
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;
----------------
xazax.hun wrote:
> li.zhe.hua wrote:
> > 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.
> Thanks for the detailed explanation, this sounds good to me! Could you add a short summary as a comment to `ignoreExprWithCleanups`?
Thank you for the explanation! This looks good to me too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124807