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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
