vsavchenko marked 5 inline comments as done.
vsavchenko added inline comments.
================
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)};
+ }
----------------
ASDenysPetrov wrote:
> 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.
It is a valid point, I will add this test and change this behaviour!
The name is confusing indeed, maybe you have any ideas what would be more
appropriate?
================
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());
----------------
ASDenysPetrov wrote:
> As for me, the last //reason// fully covers previous special cases, so you
> can omit those ones, thus simplify the comment.
I really want to be clear about the first two cases to explain why this works
for any sign of `From` and `To`
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:633
+
+ // Check if LHS is 0. It's a special case when the result is guaranteed
+ // to be 0 no matter what RHS is (we put to the side the case when RHS is
----------------
xazax.hun wrote:
> I wonder if we actually need this? I vaguely recall that we are doing a lot
> of simplifications during building symbolic expressions. I would be surprised
> if this identity is not handled there. (And in that case, probable this
> should be added there.) Or we might need a comment to explain why do we need
> this simplification at both places.
Yeah, we don't do it in `SValBuilder`, but it is definitely a better place for
that particular case. I'll move it.
================
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()) {
----------------
ASDenysPetrov wrote:
> Extend the comment, please, why we should move bounds to zero at all.
Good point!
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:684
+
+ return {RangeFactory, ValueFactory.getValue(Min),
ValueFactory.getValue(Max)};
+}
----------------
ASDenysPetrov wrote:
> 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?
Yes, it is a conservative range for any ranges because only the sign of the
operation is specific to different implementations
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80117/new/
https://reviews.llvm.org/D80117
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
