This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Ensure well-formed flow conditions.
ClosedPublic

Authored by ymandel on Apr 15 2022, 8:10 AM.

Details

Summary

Ensure that the expressions associated with terminators are associated with a
value. Otherwise, we can generate degenerate flow conditions, where both
branches share the same condition.

Diff Detail

Event Timeline

ymandel created this revision.Apr 15 2022, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 8:10 AM
ymandel requested review of this revision.Apr 15 2022, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 8:10 AM
sgatev added inline comments.Apr 19 2022, 4:22 AM
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
114–128

What does it mean for flow conditions to be mutually exclusive? Why is this a requirement?

125–138

Let's also add tests for these two paths.

ymandel updated this revision to Diff 423682.Apr 19 2022, 11:10 AM

address comments; move tests to proper locations

ymandel marked an inline comment as done.Apr 19 2022, 11:18 AM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
125–138

Turns out this code was overkill -- the conditions are rvalues (even lvalues have an implicit cast inserted), so regardless of any pre-existing form, we should just replace it unconditionally with a simple boolean representation.

I've added a new test, though, for fields (vs the existing one for functions), which should exercise the path of Loc != nullptr, vs the function which (with current implementation) will exercise Loc == nullptr.

xazax.hun accepted this revision.Apr 19 2022, 11:18 AM
xazax.hun added inline comments.
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
110

I am still not convinced why would we ever need SkipPast, as the AST will always have the necessary LValueToRValue conversions when we need to look past references. But this is unrelated to this patch.

119

I think this get or create pattern is getting more and more widespread. At some point we might want a helper.

This revision is now accepted and ready to land.Apr 19 2022, 11:18 AM
ymandel updated this revision to Diff 423690.Apr 19 2022, 11:32 AM

add comments to tests.

ymandel updated this revision to Diff 423693.Apr 19 2022, 11:37 AM
ymandel marked 2 inline comments as done.

added fixme

Thanks for the review!

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

Agreed. But, I tried to pull out the SkipPast and it crashed (hence the FIXME). I'll try to track down what's happening, but it may be related to our conflation of ReferenceValue for normal lvalues and reference-typed declarations. Regardless, we want to reconsider our treatment of the different value classes.

119

agreed. Added fixme.

sgatev accepted this revision.Apr 20 2022, 6:53 AM
sgatev added inline comments.
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
884

What does "abstract" mean here? Perhaps "arbitrary"?

ymandel updated this revision to Diff 423945.Apr 20 2022, 9:55 AM

address comments

ymandel marked an inline comment as done.Apr 20 2022, 9:56 AM
This revision was landed with ongoing or failed builds.Apr 20 2022, 10:02 AM
This revision was automatically updated to reflect the committed changes.