Page MenuHomePhabricator

[analyzer] PR37501: Disable the assertion for reverse-engineering logical op short circuits.
ClosedPublic

Authored by NoQ on Mar 26 2019, 5:56 PM.

Details

Summary

Replace the incorrect assertion with a lot of swearing in comments. This should take care of these rare crashes such as https://bugs.llvm.org/show_bug.cgi?id=37501 and the minimal fallback behavior i added should hopefully be better than just dropping the assertion (instead of picking a path for backtracking at random, we admit that we don't know the value). The idea behind the original implementation is elegant, but it doesn't really work.

I did try to replace the logic with trying to evaluate logical operators as if they were regular operators: take the LHS value, take the RHS value, apply math. It's not easy because it requires tweaking liveness analysis to make sure that all the necessary values are alive; i couldn't immediately fix all the failres on tests. Even if i did get it working, i suspect that this is also a dead end, because we have these "unknown value" things. Even if the values of the operands were too complicated to compute and evaluated to UnknownVals, we can still obtain the correct value of the logical operator via backtracking ("we're at this point in our execution path, therefore the LHS must have been assumed to be true"), but not via math ("whoops, the LHS is unknown").

We could combine both techniques (backtracking and math, use one when the other fails), but that's an overkill for such a rare problem, so i decided to just remove the assertion.

The truly reliable solution in our situation would be to set a flag in the program state whenever we're doing a short circuit operation, and consume it in VisitLogicalExpr. But that also requires making sure we don't mess up with all those CFG cornercases, so i decided to leave it as an exercise for the future generations of Static Analyzer developers.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Mar 26 2019, 5:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 5:56 PM

There is no connection with my reverse-engineering reverted patch: https://reviews.llvm.org/D57410?id=184992 ? It evaluates the left and the right side and may it could help.

This revision is now accepted and ready to land.Mar 27 2019, 2:12 AM
This revision was automatically updated to reflect the committed changes.