This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Centralize expression skipping logic
ClosedPublic

Authored by li.zhe.hua on May 4 2022, 2:47 PM.

Details

Summary

A follow-up to 62b2a47 to centralize the logic that skips expressions
that the CFG does not emit. This allows client code to avoid
sprinkling this logic everywhere.

Add redirects in the transfer function to similarly skip such
expressions by forwarding the visit to the sub-expression.

Diff Detail

Event Timeline

li.zhe.hua created this revision.May 4 2022, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 2:47 PM
li.zhe.hua requested review of this revision.May 4 2022, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 2:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ymandel accepted this revision.May 5 2022, 11:09 AM

Thanks!

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
368

maybe bind to a temporary to avoid doing this twice (in debug code, that is)? arguably not that important to optimize debug mode but may also improve readability.

clang/lib/Analysis/FlowSensitive/Transfer.cpp
38–41

Is the idea that the skipping is now built into getValue via getStorageLocation?

545–561

I thought this is the default behavior?

This revision is now accepted and ready to land.May 5 2022, 11:09 AM
li.zhe.hua marked 3 inline comments as done.

Address reviewer comments

clang/lib/Analysis/FlowSensitive/Transfer.cpp
38–41

I don't believe I fixed this. d002495 addressed this by having integral casts route to the sub-expression.

545–561

The default behavior of StmtVisitor is that VisitFoo will call VisitFooBase where Foo derives from FooBase (e.g. the default implementation of VisitExprWithCleanups calls VisitFullExpr). The base case is a VisitStmt implementation that does approximately nothing, especially if RetTy = void.

So in this case, I'm changing this to automatically ignore ParenExpr and ExprWithCleanups and visit the sub-expression, which is not the default behavior.

This isn't used when we call the transfer function from CFG-provided statements, because the CFG doesn't emit these nodes. However, this is relevant when we manually call the transfer function, e.g. from here.

ymandel added inline comments.May 5 2022, 1:00 PM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
38–41

Great. Just to be sure I understand: you mean that the IgnoreCasts() calls were already redundant because of d002495?

545–561

Interesting. Thanks for the explanation!

li.zhe.hua marked an inline comment as done.May 5 2022, 1:22 PM
li.zhe.hua added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
38–41

Correct, that's my understanding. This diff hunk could be separately committed first. (And maybe it should, for blame clarity.)

li.zhe.hua marked an inline comment as done.May 5 2022, 1:25 PM
This revision was landed with ongoing or failed builds.May 5 2022, 1:41 PM
This revision was automatically updated to reflect the committed changes.