This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Extend transfer functions for other `CFGElement`s
ClosedPublic

Authored by wyt on Aug 10 2022, 1:04 PM.

Details

Summary

Previously, the transfer function void transfer(const Stmt *, ...) overriden by users is restricted to apply only on CFGStmts and its contained Stmt.

By using a transfer function (void transfer(const CFGElement *, ...)) that takes a CFGElement as input, this patch extends user-defined analysis to all kinds of CFGElement. For example, users can now handle CFGInitializers where CXXCtorInitializer AST nodes are contained.

Diff Detail

Event Timeline

wyt created this revision.Aug 10 2022, 1:04 PM
Herald added a project: Restricted Project. · View Herald Transcript
wyt requested review of this revision.Aug 10 2022, 1:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:04 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
wyt edited the summary of this revision. (Show Details)Aug 10 2022, 1:16 PM
wyt added reviewers: hlopko, gribozavr2, ymandel, sgatev, samestep.
wyt edited the summary of this revision. (Show Details)Aug 11 2022, 1:03 AM
gribozavr2 added inline comments.Aug 11 2022, 4:00 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
103

Instead of adding virtual function, please continue following the CRTP pattern. Add the expected function declarations in the comment above the class.

108
112

Ditto.

134
144

"PostVisitCFG"?

209

PTAL at the intended usage of getAs() here: llvm-project/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

That is, it should be used like dyn_cast in if statements.

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
260
261

"CfgStmt" is violating the style guide casing rules.

270–271

Ditto.

325

Should be using castAs().

329

castAs()

438

Please add the newline.

wyt updated this revision to Diff 452957.Aug 16 2022, 4:54 AM
wyt marked 9 inline comments as done.

Address comments.

wyt updated this revision to Diff 452964.Aug 16 2022, 5:22 AM

Add FIXME.

wyt added inline comments.
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
103

Instead of adding virtual function, please continue following the CRTP pattern. Add the expected function declarations in the comment above the class.

As discussed, moving to a CRTP pattern causes the code to break as they have not been implemented by users yet. Added a FIXME to do so after users have been migrated to implement transferCFGElement.

wyt edited the summary of this revision. (Show Details)Aug 16 2022, 5:24 AM
gribozavr2 accepted this revision.Aug 16 2022, 8:16 AM
gribozavr2 added inline comments.
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
139
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
167
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
81

Please use triple slash for doc comments.

83
This revision is now accepted and ready to land.Aug 16 2022, 8:16 AM
wyt updated this revision to Diff 453702.Aug 18 2022, 10:36 AM
wyt marked 2 inline comments as done.

Address comments. Unable to rename check/runDataflowOnCFG to check/runDataflow currently due to ambiguous use.

gribozavr2 accepted this revision.Aug 18 2022, 11:33 AM
wyt added inline comments.Aug 19 2022, 1:07 PM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
139

Cannot be renamed currently, as the use between the function that applies to CFGElement and CFGStmt is ambiguous. Added a FIXME to rename after we have updated the implementation for users and remove the function that acts on CFGStmts.

clang/unittests/Analysis/FlowSensitive/TestingSupport.h
83

Same as above, using the same name for the function that applies to CFGElement and CFGStmt is ambiguous.

sgatev added inline comments.Aug 22 2022, 3:00 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
102

Any reason not to call this one transfer too?

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
157

Let's start struct and member comments with ///.

179
307
309
323–335

The level of abstraction for built-in and user code is different in this function which makes it hard to follow. Let's move this in a builtinTransfer(const CFGElement &, TypeErasedDataflowAnalysisState &) function that dispatches to one of the other built-in transfer functions. This way transferCFGBlock won't mix implementation details with the high level algorithm.

336–342
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
161–167
wyt updated this revision to Diff 454523.Aug 22 2022, 8:52 AM
wyt marked 8 inline comments as done.

Address comments.

wyt updated this revision to Diff 454532.Aug 22 2022, 9:06 AM

Rebase.

gribozavr2 accepted this revision.Aug 22 2022, 9:11 AM
sgatev accepted this revision.Aug 23 2022, 12:58 AM
thakis added a subscriber: thakis.Aug 26 2022, 3:33 PM

I don't know how you commited this, but the commit message is missing all the (great!) details that are here on the review. See here: https://github.com/llvm/llvm-project/commit/4b815eb4fde0202434c6 or here: https://reviews.llvm.org/rG4b815eb4fde0202434c63

Please update your commit workflow so that the description on phab makes it into the commit going forward :)

wyt added a comment.Aug 26 2022, 3:38 PM

@thakis
Thanks for pointing that out, will revert and recommit - since it also ran into some build failures.

wyt reopened this revision.Aug 26 2022, 3:43 PM
This revision is now accepted and ready to land.Aug 26 2022, 3:43 PM
wyt updated this revision to Diff 456049.Aug 26 2022, 4:01 PM

Mark override for virtual transfer function.

gribozavr2 accepted this revision.Aug 26 2022, 4:35 PM
wyt updated this revision to Diff 456624.Aug 30 2022, 5:23 AM

Replace use of virtual functions with sfinea and CRTP to call transfer functions defined by the user.

wyt edited the summary of this revision. (Show Details)Aug 30 2022, 5:34 AM
gribozavr2 accepted this revision.Aug 30 2022, 5:43 AM