This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by li.zhe.hua on May 2 2022, 3:03 PM.

Details

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.

Diff Detail

Event Timeline

li.zhe.hua created this revision.May 2 2022, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 3:03 PM
li.zhe.hua requested review of this revision.May 2 2022, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 3:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
li.zhe.hua updated this revision to Diff 426528.May 2 2022, 3:08 PM

Remove else after return

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 updated this revision to Diff 426711.May 3 2022, 8:08 AM

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

sgatev added inline comments.May 3 2022, 8:52 AM
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 added inline comments.May 3 2022, 9:33 AM
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 added inline comments.May 3 2022, 9:40 AM
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 added inline comments.May 3 2022, 10:40 AM
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

Handle FullExprs in the transfer function.

li.zhe.hua updated this revision to Diff 426823.May 3 2022, 1:49 PM

Switch from implementing in the trasfer function to skipping ExprWithCleanups.

li.zhe.hua marked 4 inline comments as done.May 3 2022, 1:50 PM
li.zhe.hua added inline comments.
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.

li.zhe.hua retitled this revision from [clang][dataflow] Avoid assert for invalid cast to BoolValue to [clang][dataflow] Only skip ExprWithCleanups when visiting terminators.May 3 2022, 1:51 PM
li.zhe.hua edited the summary of this revision. (Show Details)
xazax.hun accepted this revision.May 3 2022, 2:26 PM
xazax.hun added inline comments.
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?

This revision is now accepted and ready to land.May 3 2022, 2:26 PM
li.zhe.hua updated this revision to Diff 426860.May 3 2022, 3:16 PM
li.zhe.hua edited the summary of this revision. (Show Details)

Expand ignoreExprWithCleanups comment with more context.

li.zhe.hua marked an inline comment as done.May 3 2022, 3:50 PM
ymandel accepted this revision.May 3 2022, 7:33 PM

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 added inline comments.May 4 2022, 12:46 AM
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
619–620
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
77

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

li.zhe.hua updated this revision to Diff 426985.May 4 2022, 6:31 AM
li.zhe.hua marked 7 inline comments as done.

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 updated this revision to Diff 426987.May 4 2022, 6:34 AM

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

sgatev accepted this revision.May 4 2022, 6:59 AM

Looks great! Thank you!

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
1156

Maybe call this PointerToBoolImplicitCast and remove the FIXME?