This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Extend flow conditions from block terminators
ClosedPublic

Authored by sgatev on Mar 4 2022, 3:06 AM.

Details

Summary

This is part of the implementation of the dataflow analysis framework.
See "[RFC] A dataflow analysis framework for Clang AST" on cfe-dev.

Diff Detail

Event Timeline

sgatev created this revision.Mar 4 2022, 3:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 3:06 AM
sgatev requested review of this revision.Mar 4 2022, 3:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 3:06 AM
sgatev updated this revision to Diff 412969.Mar 4 2022, 3:14 AM

Minor tweaks.

xazax.hun added inline comments.Mar 4 2022, 11:19 AM
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
99

MergedEnv? Also, the documentation above gives a short description of the relationship between Val1, Val2, and MergedVal. But it gives little pointers what are we supposed to do with the Environment.

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

Is this actually correct? Or did you mean Conjunction?

xazax.hun added inline comments.Mar 4 2022, 4:14 PM
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
123

Oh, sorry. Just realized the empty set is interpreted as true. In this case this is correct.

ymandel accepted this revision.Mar 6 2022, 10:30 AM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
154

The current comment gets at the big picture, but focusing on the actual disjunction that is being guarded (that is, just the remaining, unshared clauses) may be better. So, maybe instead point out that "True v (X ^ ...)" is equivalent to "True", since if either val is nullptr it represents "true"?

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

Can we be more specific? I'd think we need to invert for specifically the successor corresponding to "else" or what not.

This revision is now accepted and ready to land.Mar 6 2022, 10:30 AM
sgatev updated this revision to Diff 413426.Mar 7 2022, 4:23 AM
sgatev marked 5 inline comments as done.

Address reviewers' comments.

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
99

Updated the name and added some pointers in the documentation.

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

I added a general comment at the top and modified the comment here to target the specific case.

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

Yup. Updated the comment to be more specific.

xazax.hun accepted this revision.Mar 7 2022, 9:25 AM

Thanks, with the new comments it is much clearer what is going on!

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

We only expect || and && to be terminators right? I wonder if we should add an assert so we get notified if the language gets extended with a new operator that we do not support yet.

sgatev updated this revision to Diff 413523.Mar 7 2022, 9:35 AM
sgatev marked an inline comment as done.

Address reviewers' comments.

sgatev added inline comments.Mar 7 2022, 9:37 AM
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
89

Right. Adding an assert here sounds good to me.

This revision was landed with ongoing or failed builds.Mar 7 2022, 9:52 AM
This revision was automatically updated to reflect the committed changes.