[PATCH] D112621: [analyzer][solver] Introduce reasoning for not equal to operator

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I haven't looked at the implementation. I only checked the tests and something is not quite right there. See my comments inline.
BTW the line-coverage is good. I found only two branches which I want to have tests for - see inline.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1624
+  if (LHS.isEmpty() || RHS.isEmpty())
+    return RangeFactory.getEmptySet();
+
----------------
This branch is uncovered by tests.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1633
+    APSIntType CastingType(std::max(LHS.getBitWidth(), RHS.getBitWidth()),
+                           LHS.isUnsigned() || RHS.isUnsigned());
+
----------------
Although the `RHS.isUnsigned()` is covered 2 times in the `check-clang-analysis`target, I think it would make sense to have a dedicated test for this case in the `constant-folding.c`.


================
Comment at: clang/test/Analysis/constant-folding.c:289-301
+  if (u1 > INT_MAX && u1 <= UINT_MAX / 2 + 4 && u1 != UINT_MAX / 2 + 2 &&
+      u1 != UINT_MAX / 2 + 3 && s1 >= INT_MIN + 1 && s1 <= INT_MIN + 2) {
+    // u1: [INT_MAX+1, INT_MAX+1]U[INT_MAX+4, INT_MAX+4],
+    // s1: [INT_MIN+1, INT_MIN+2]
+    clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+  }
+
----------------
These two hunks seem to be the same. We should probably keep one.
In addition to that, clang thinks the answer for `u1 != s1` depends on the operands.
We should probably return `UNKNOWN` for such cases. https://godbolt.org/z/aG1oY5Mr4


================
Comment at: clang/test/Analysis/constant-folding.c:315-319
+  if (s1 < 1 && s1 > -6 && s1 != -4 && s1 != -3 &&
+      u1 > UINT_MAX - 4 && u1 < UINT_MAX - 1) {
+    // s1: [-5, -5]U[-2, 0], u1: [UINT_MAX - 3, UINT_MAX - 2]
+    clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+  }
----------------
Clang thinks it depends on the operands if `u1 != s1` is true.
https://godbolt.org/z/Pf3eYKnzd


================
Comment at: clang/test/Analysis/constant-folding.c:321-325
+  if (s1 < 1 && s1 > -7 && s1 != -4 && s1 != -3 &&
+      u1 > UINT_MAX - 4 && u1 < UINT_MAX - 1) {
+    // s1: [-6, -5]U[-2, 0], u1: [UINT_MAX - 3, UINT_MAX - 2]
+    clang_analyzer_eval(u1 != s1); // expected-warning{{TRUE}}
+  }
----------------
Clang thinks it depends on the operands if `u1 != s1` is true.
https://godbolt.org/z/1YYcd1EY8


================
Comment at: clang/test/Analysis/constant-folding.c:328-332
+  if (((u1 >= 1 && u1 <= 2) || (u1 >= 8 && u1 <= 9)) &&
+      u2 >= 5 && u2 <= 6) {
+    // u1: [1, 2]U[8, 9], u2: [5, 6]
+    clang_analyzer_eval(u1 != u2); // expected-warning{{TRUE}}
+  }
----------------
Clang thinks it depends on the operands if `u1 != u2` is true.
I believe, in such cases we should have returned `UNKNOWN`: https://godbolt.org/z/fxaT7YYob


================
Comment at: clang/test/Analysis/constant-folding.c:404-407
+  if (uch > 1 && sch < 1) {
+    // uch: [2, CHAR_MAX], sch: [SCHAR_MIN, 0]
+    clang_analyzer_eval(uch != sch); // expected-warning{{UNKNOWN}}
+  }
----------------
Shouldn't it print `TRUE`?
Clang will optimize this to `return true`. https://godbolt.org/z/6c6EPYofY
We should probably conclude the same here.


================
Comment at: clang/test/Analysis/constant-folding.c:420-423
+  if (ush > 1 && ssh < 1) {
+    // ush: [2, USHRT_MAX], ssh: [SHRT_MIN, 0]
+    clang_analyzer_eval(ush != ssh); // expected-warning{{UNKNOWN}}
+  }
----------------
It should return `TRUE`. https://godbolt.org/z/44Pax16d1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112621