This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add flow condition constraints to Environment
ClosedPublic

Authored by sgatev on Mar 1 2022, 3:21 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 1 2022, 3:21 AM
sgatev requested review of this revision.Mar 1 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 3:21 AM
sgatev updated this revision to Diff 412034.Mar 1 2022, 3:31 AM

Minor changes to comments.

ymandel accepted this revision.Mar 1 2022, 9:08 AM
ymandel added inline comments.
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
41

I think S would be easier to read.

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
244–245
253–254
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
35

Why not use try_emplace earlier, to save of one of the lookups?

clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp
29–32

maybe add another one which simply tests that two calls to createAtomicBoolValue return distinct values (or even just distinct pointers?)

This revision is now accepted and ready to land.Mar 1 2022, 9:08 AM
xazax.hun added inline comments.Mar 1 2022, 9:52 AM
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
41

Initially, I was wondering if we want to make the solver optional to potentially speed up analyses that do not need this capability. But I realized, we could just pass in a dummy no-op solver in those cases so this should be ok.

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

Should we move this check to DataflowAnalysisContext to protect against cases where users inadvertently try to call the its methods directly instead of using the Environment?

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
31

Alternatively, we could canonicalize BoolValues, e.g. always having the operand with the smaller address on the left. This could help us doing one less lookup (at the cost of a pointer comparison and sometimes a swap).

sgatev updated this revision to Diff 412160.Mar 1 2022, 10:39 AM
sgatev marked 8 inline comments as done.

Address reviewers' comments.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
41

Agreed.

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

Done, but kept the implication and bi-conditional helpers in Environment as I'm not sure if we'll need them in DataflowAnalysisContext at this point.

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
31

Agreed. I updated the implementation.

35

Makes sense. I updated the implementation.

clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp
29–32

Done.

sgatev updated this revision to Diff 412162.Mar 1 2022, 10:44 AM

Address reviewers' comments.

ymandel accepted this revision.Mar 1 2022, 10:55 AM
xazax.hun accepted this revision.Mar 1 2022, 11:06 AM
xazax.hun added inline comments.
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
43

As far as I understand, we need to do the double lookup (find and later try_emplace) because we do not want to allocate + take ownership when we hit the cache. An alternative is to first do try_emplace with a nullptr and overwrite the value if the emplace was successful. But I guess that also can be confusing, so it is up to you which one would you prefer.

ymandel added inline comments.Mar 1 2022, 11:26 AM
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
43

good point. I was thinking of the value case. Sorry.

sgatev updated this revision to Diff 412256.Mar 1 2022, 2:38 PM

Address reviewers' comments.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 2:38 PM
sgatev marked 2 inline comments as done.Mar 1 2022, 2:40 PM

Thanks!

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
43

Makes sense and I think it doesn't look that bad so let's go with it.

xazax.hun accepted this revision.Mar 1 2022, 2:51 PM

Thanks, this looks wonderful!

This revision was landed with ongoing or failed builds.Mar 2 2022, 12:58 AM
This revision was automatically updated to reflect the committed changes.