[PATCH] [analyzer][Bugfix/improvement] Fix for PR16833

[Jordan Rose]

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1691
@@ +1690,3 @@
+      defaultIsFeasible = false;
+      break;
+    }
----------------
a.sidorin wrote:
> jordan_rose wrote:
> > I don't think this `break` is correct. Cases are not guaranteed to be in order.
> Does the order really affect this code? The fact that DefaultSt is null means that all possible cases for switch were already covered (range-by-range), there is nothing left to consider and we can break out of loop. Or am I missed something?
Hm, that's true—that means a case matched definitively. Okay, I stand corrected.

================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:604-605
@@ +603,4 @@
+                                            const llvm::APSInt& Adjustment) {
+  ProgramStateRef St1 = assumeSymLT(state, sym, From, Adjustment);
+  ProgramStateRef St2 = assumeSymGT(state, sym, To, Adjustment);
+  RangeSet Range1 = St1 ? GetRange(St1, sym) : F.getEmptySet();
----------------
a.sidorin wrote:
> jordan_rose wrote:
> > These are expensive operations. Is it possible to do this at a lower level? Or are there good reasons to do this extra work?
> > 
> > One reason I'm not so happy with this is that some day we could have constraints on one symbol affect another symbol, but this doesn't preserve that. Then again, neither would a lower-level solution.
> Jordan, could you explain what do you mean here?
> If you mean removing of creation of new states (creating ranges only), that could be done easily.
> But it seems that you mean something else that I cannot understand correctly.
> BTW, does the code in previous function need correction too?
The creation of new states is the expensive part. I'm kind of okay with the previous function doing it because it keeps things simple, it does exactly what it's specified to do, and it only has one intermediate state. (But yes, avoiding that would be nice if it can still be done tidily.)

For the second, I guess I was concerned about a hypothetical constraint manager that can handle things like this:

Given: ($x * 2) < 20 - //we do handle this today, opaquely//
Assume: $x outside of (0, 10)
Then: ($x * 2) < 0

Exactly what $x is is still not known because the analyzer operates under -fwrapv semantics. But we //could// make the above assumption about ($x * 2).

That said, we don't do anything like that today, and this is within RangeConstraintManager, not a general-purpose implementation in ConstraintManager or SimpleConstraintManager. So maybe this is all right, though I still think it would be a good idea to do it at the RangeSet level rather than the ProgramState level.

http://reviews.llvm.org/D5102