Page MenuHomePhabricator

[Sema] SequenceChecker: Fix handling of operator ||, && and ?:
AcceptedPublic

Authored by riccibruno on Feb 5 2019, 5:25 AM.

Details

Summary

The current handling of the operators ||, && and ?: has a number of false positive and false negative. The issues for operator || and && are:

  1. We need to add sequencing regions for the LHS and RHS as is done for the comma operator. Not doing so causes false positives in expressions like ((a++, false) || (a++, false)); (from PR39779, see PR22197 for another example).
  1. In the current implementation when the evaluation of the LHS fails, the RHS is added to a worklist to be processed later. This results in false negatives in expressions like (a && a++) + a.

Fix these issues by introducing sequencing regions for the LHS and RHS, and by not deferring the visitation of the RHS.

The issues with the ternary operator ?: are similar, with the added twist that we should not warn on expressions like (x ? y += 1 : y += 2) since exactly one of the 2nd and 3rd expression is going to be evaluated, but we should still warn on expressions like (x ? y += 1 : y += 2) = y.

Diff Detail

Repository
rC Clang

Event Timeline

riccibruno created this revision.Feb 5 2019, 5:25 AM

Looking at git blame, this change means that we will warn on (b && x++) + (!b && x++). However we will also warn on an expression like (x > some_constant && y++) + (x < some_constant && y++). This seems to be the job of a static analyser which is able to do some control flow analysis. Moreover for this warning it seems to me that having some false negatives is worse than having some false positive. As a data point I ran this on the entire LLVM codebase and on all of Boost, and did not find any additional warning.

rsmith accepted this revision.Feb 5 2019, 9:03 AM

The "false negatives" in the current behaviour are the result of an intentional decision to avoid false positives for unsequenced operations that cannot actually both happen as part of the same evaluation (because both are conditional on different and mutually-exclusive conditions), such as the false positive we now get in the tests. I think it's reasonable to revisit that decision, though; the new false positive cases do not realistically seem likely to occur in real code whereas the false negatives do.

test/Sema/warn-unsequenced.c
79 ↗(On Diff #185284)

Please at least add a FIXME for this false positive.

This revision is now accepted and ready to land.Feb 5 2019, 9:03 AM
riccibruno marked an inline comment as done.

Rebased

Rebased on D57660. No need to look at it.