[PATCH] D80117: [analyzer] Introduce reasoning about symbolic remainder operator

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko
I've made some assumptions.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:459-469
+    if (Origin.From().isMinSignedValue()) {
+      // If mini is a minimal signed value, absolute value of it is greater
+      // than the maximal signed value.  In order to avoid these
+      // complications, we simply return the whole range.
+      return {ValueFactory.getMinValue(RangeType),
+              ValueFactory.getMaxValue(RangeType)};
+    }
----------------
I think you should swap `if` statements. I'll explain.
Let's consider the input is an **uint8** range [42, 242] and you will return [0, 242] in the second `if`.
But if the input is an **uint8** range [128, 242] you will return [0, 255] in the first `if`, because 128 is an equivalent of -128(INT8_MIN) in binary representation so the condition in the first if would be true.
What is the great difference between [42, 242] and [128, 242] to have different results? Or you've just missed this case?

P.S. I think your function's name doesn't fit its body, since //absolute value// is always positive (without sign) from its definition, but you output range may have negative values. You'd better write an explanation above the function and rename it.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:481
+    //   * Otherwise, From <= 0, To >= 0, and
+    //     AbsMax == max(abs(From), abs(To))
+    llvm::APSInt AbsMax = std::max(-Origin.From(), Origin.To());
----------------
As for me, the last  //reason// fully covers previous special cases, so you can omit those ones, thus simplify the comment.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:659
+  //
+  // If we are dealing with unsigned case, we shouldn't move the lower bound.
+  if (Min.isSigned()) {
----------------
Extend the comment, please, why we should move bounds to zero at all.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:684
+
+  return {RangeFactory, ValueFactory.getValue(Min), ValueFactory.getValue(Max)};
+}
----------------
Is it OK to return this rangeset in case when one of operands(or both) is negative, since this rangeset can vary from specific implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80117