[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 9 inline comments as done.
gamesh411 added a comment.

As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.



================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:88-89
+          new BuiltinBug(this, "Enum cast out of range",
+                         "The value provided to the cast expression is not in "
+                         "the valid range of values for the enum."));
+    C.emitReport(llvm::make_unique<BugReport>(
----------------
NoQ wrote:
> I believe that we'd need to explain the range we're seeing for the value in this case. Probably by asking the constraint manager to dump the range of possible values through some new API call (in case of `RangeConstraintManager` it should be straightforward).
> 
> Because otherwise it may be unobvious from the path what values are possible, when the `SVal` is a symbol.
> 
> Or, even better, we could highlight, with the help of a bug reporter visitor, the places where constraints were added to the symbol (then, again, it'd be better with ranges).
> 
> Ideally:
> ```
> enum { Zero=0, One, Two, Three, Four } y;
> 
> void foo(int x) {
>   if (x > 3) {
>     // core note: Assuming 'x' is greater than 3
>     // checker note: The assigned value is assumed to be within range [4, 2147483647]
>     ...
>   }
>   z = x;
>   // intermix of 'x' and 'z' is intentional:
>   // checker notes should track symbol, not variable,
>   // which is what makes them great.
>   if (z < 7) {
>     // core note: Assuming 'z' is less than 7
>     // checker note: The assigned value is assumed to be within range [4, 6]
>     if (x > 4) {
>       // core note: Assuming 'x' is greater than 4
>       // checker note: The assigned value is assumed to be within range [5, 6]
>       y = z; // warning: The value provided to the cast expression is in range [5, 6], which is not in the valid range of values for the enum
>     }
>   }
> }
> ```
> (with a special case when the range consists of one element)
As far as I know the current reporting system, and the ConstraintManager API does not allow for this degree of finesse when it comes to diagnostics. It is however a good idea worth pursuing as part of the enhancement of the aforementioned subsystems.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:119-122
+  // Check if any of the enum values possibly match.
+  bool PossibleValueMatch =
+      std::any_of(DeclValues.begin(), DeclValues.end(),
+                  ConstraintBasedEQEvaluator(C, *ValueToCastOptional));
----------------
NoQ wrote:
> I suspect that enum values often cover a whole segment, and in this case a pair of assumes for the sides of the segment (or, even better, a single `assumeInclusiveRange`) would be much faster.
I have cosidered assumeInclusiveRange, however there can be enums with holes in represented values (for example the the enums in the first part of the test cases). In such case it would not be practical to call assumeInclusiveRange for all subranges.


https://reviews.llvm.org/D33672