[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1397
+        (LHS.To() < 0 && RHS.To() < 0 && Max > 0) ||
+        (LHS.To() < 0 && RHS.To() < 0 && Max > 0)) {
+      return {RangeFactory, Tmin, Tmax};
----------------
This clause is exactly the same as the previous one, it is a mistake.
And I think we should have a test that could've shown that.
Also, since you are checking for overflows for both the beginning and the end, we should have tests where both overflow.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1401-1402
+
+    // FIXME: This case in particular is resulting in failed assertion.
+    Range First = Range(Tmin, Max);
+    Range Second = Range(Min, Tmax);
----------------
manas wrote:
> I changed the logic from using getCrucialPoints (which I mentioned in the e-mail thread) to simple checks to determine overflows using signedness. But the failed assertions again popped up. And I was unable to pin-point the issue here. Can someone help me?
> 
> Although, I am thinking of revising the above logic by using bitwise methods to detect overflows.
This is actually one place that I won't expect an assertion failure.
Can we get a bit more detail on it?  Is it again `From > To` (which will indeed be weird) or something different?

NOTE: Asking for help (either here or on StackOverflow) is a lot like filing a bug report, try to put as much information as possible, try to structure this information so it is easy to follow.  It's also good to tell people what you tried, instead of saying that you tried something and it didn't work.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1409-1415
+  if ((LHS.From() > 0 && RHS.From() > 0 && Min < 0) ||
+      (LHS.From() < 0 && RHS.From() < 0 && Min > 0) ||
+      (LHS.To() > 0 && RHS.To() > 0 && Max < 0) ||
+      (LHS.To() < 0 && RHS.To() < 0 && Max > 0)) {
+    // return [Tmin, Tmax]
+    return {RangeFactory, Tmin, Tmax};
+  }
----------------
I thought we talked quite a lot that there is nothing bad with overflows and here we have that if ANY overflow happened, we bail out and don't give any result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440