[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867021 <https://reviews.llvm.org/D103096#2867021>, @ASDenysPetrov wrote:

> In D103096#2866730 <https://reviews.llvm.org/D103096#2866730>, @vsavchenko wrote:
>
>> In D103096#2866704 <https://reviews.llvm.org/D103096#2866704>, @ASDenysPetrov wrote:
>>
>>> @vsavchenko
>>
>> That's not the question I'm asking.  Why do you need to set constraints for other symbolic expressions, when `SymbolicInferrer` can look them up on its own?  Which cases will fail if we remove that part altogether?
>
> I see. Here is what fails in case if we don't update other constraints:
>
>   void test(int x) {
>     if ((char)x > -10 && (char)x < 10) {
>       if ((short)x == 8) {
>         // If you remove updateExistingConstraints,
>         // then `c` won't be 8. It would be [-10, 10] instead.
>         char c = x;
>         if (c != 8)
>           clang_analyzer_warnIfReached(); // should no-warning, but fail
>       }
>     }
>   }

OK, it's something! Good!
I still want to hear a good explanation why is it done this way.  Here `c` is mapped to `(char)x`, and we have `[-10, 10]` directly associated with it, but we also have `(short)x` associated with `[8, 8]`.  Why can't `VisitSymbolCast` look up constraints for `(short)x` it already looks up for constraints for different casts already.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits or... I'm not sure I fully understand.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1215
+      QualType T = RootSym->getType();
+      if (!T->isIntegralOrEnumerationType())
+        return VisitSymExpr(Sym);
----------------
Can we get a test for that?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1216
+      if (!T->isIntegralOrEnumerationType())
+        return VisitSymExpr(Sym);
+
----------------
Same goes here.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+      // Find the first constraint and exit the loop.
+      RSPtr = getConstraint(State, S);
+    }
----------------
Why do you get associated constraint directly without consulting with what `SymbolRangeInferrer` can tell you about it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103096/new/

https://reviews.llvm.org/D103096