This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add support for (built-in) (in)equality operators
ClosedPublic

Authored by ymandel on Mar 31 2022, 8:22 AM.

Details

Summary

Adds logical interpretation of built-in equality operators, == and !=.

Diff Detail

Event Timeline

ymandel created this revision.Mar 31 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 8:22 AM
ymandel requested review of this revision.Mar 31 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 8:22 AM
ymandel edited the summary of this revision. (Show Details)Mar 31 2022, 8:24 AM
xazax.hun accepted this revision.Mar 31 2022, 9:23 AM
xazax.hun added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
56

I think a possible alternative way to handle this is to do some sort of "unification" for the two values. The current approach is superior in the sense that solving and generating constraints are clearly separate. Also this might be better for generating useful error messages. In case the solver ever becomes a bottleneck, I wonder whether replacing this with a unification step would speed things up a bit.
Although that would only work for the equality case.

This revision is now accepted and ready to land.Mar 31 2022, 9:23 AM
ymandel marked an inline comment as done.Mar 31 2022, 9:53 AM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
56

That's a good point. Although I wonder if we could get the best of both worlds by adding a unification pre-step to the solver itself, basically looking for iff patterns of this sort. Then, the solver could keep a record of such unifications which it could export for diagnostic purposes.

On a related note, another potential use of unification is in equivalence testing of environments. Instead of strict equality of flow conditions, for example, I could imagine equivalence up to unification (aka. renaming of atoms).

ymandel added a subscriber: kinu.Mar 31 2022, 10:59 AM
sgatev accepted this revision.Mar 31 2022, 10:50 PM
sgatev added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
53

Do we need to skip past references here? If so, then let's add a test that fails if these are changed to SkipPast::None.

109–113
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
2581
2616
ymandel updated this revision to Diff 419793.Apr 1 2022, 10:12 AM
ymandel marked 2 inline comments as done.

address comments

ymandel marked 3 inline comments as done.Apr 1 2022, 10:12 AM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
53

Yes -- both of the new tests fail when we don't do this. It's actually hard to construct a test that passes without this, since DeclRefExprs of boolean type are ReferenceValue.

56

I think this connects to our discussion here: https://reviews.llvm.org/D122838?id=419518#inline-1175582

This seems like another example where tracking equivalences in the environment would be beneficial.

This revision was landed with ongoing or failed builds.Apr 1 2022, 10:23 AM
This revision was automatically updated to reflect the committed changes.