[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 215433.
chrish_ericsson_atx added a comment.

Follow-up on reviewer feedback.  Changed from blacklisting LValueToRValue to whitelisting IntegralCast.  This was a good call -- additional testing with different cast kinds showed that the assertion tripped for other casts besides LValueToRValue, e.g., FloatToIntegral.  I couldn't see any casts other than Integral where the enum check seemed appropriate.  Testing with only IntegralCast enabled gave expected (correct) results.

Also reformatted the new regtest file per reviewer comments.


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

https://reviews.llvm.org/D66014

Files:
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/test/Analysis/enum-cast-out-of-range.c


Index: clang/test/Analysis/enum-cast-out-of-range.c
===================================================================
--- clang/test/Analysis/enum-cast-out-of-range.c
+++ clang/test/Analysis/enum-cast-out-of-range.c
@@ -2,33 +2,34 @@
 // RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
 // RUN:   -verify %s
 
-enum unscoped_unspecified_t {
-  unscoped_unspecified_0 = -4,
-  unscoped_unspecified_1,
-  unscoped_unspecified_2 = 1,
-  unscoped_unspecified_3,
-  unscoped_unspecified_4 = 4
+enum En_t {
+  En_0 = -4,
+  En_1,
+  En_2 = 1,
+  En_3,
+  En_4 = 4
 };
 
 void unscopedUnspecifiedCStyle() {
-  enum unscoped_unspecified_t InvalidBeforeRangeBegin = (enum unscoped_unspecified_t)(-5); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidNegativeValue1 = (enum unscoped_unspecified_t)(-4); // OK.
-  enum unscoped_unspecified_t ValidNegativeValue2 = (enum unscoped_unspecified_t)(-3); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange1 = (enum unscoped_unspecified_t)(-2); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange2 = (enum unscoped_unspecified_t)(-1); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange3 = (enum unscoped_unspecified_t)(0); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue1 = (enum unscoped_unspecified_t)(1); // OK.
-  enum unscoped_unspecified_t ValidPositiveValue2 = (enum unscoped_unspecified_t)(2); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange4 = (enum unscoped_unspecified_t)(3); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue3 = (enum unscoped_unspecified_t)(4); // OK.
-  enum unscoped_unspecified_t InvalidAfterRangeEnd = (enum unscoped_unspecified_t)(5); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
+  enum En_t Below    = (enum En_t)(-5); // expected-warning {{not in the valid range}}
+  enum En_t NegVal1  = (enum En_t)(-4); // OK.
+  enum En_t NegVal2  = (enum En_t)(-3); // OK.
+  enum En_t InRange1 = (enum En_t)(-2); // expected-warning {{not in the valid range}}
+  enum En_t InRange2 = (enum En_t)(-1); // expected-warning {{not in the valid range}}
+  enum En_t InRange3 = (enum En_t)(0);  // expected-warning {{not in the valid range}}
+  enum En_t PosVal1  = (enum En_t)(1);  // OK.
+  enum En_t PosVal2  = (enum En_t)(2);  // OK.
+  enum En_t InRange4 = (enum En_t)(3);  // expected-warning {{not in the valid range}}
+  enum En_t PosVal3  = (enum En_t)(4);  // OK.
+  enum En_t Above    = (enum En_t)(5);  // expected-warning {{not in the valid range}}
 }
 
-enum unscoped_unspecified_t unused;
+enum En_t unused;
 void unusedExpr() {
-    // following line is not something that EnumCastOutOfRangeChecker should evaluate.  checker should either ignore this line
-    // or process it without producing any warnings.  However, compilation will (and should) still generate a warning having 
-    // nothing to do with this checker.
+    // Following line is not something that EnumCastOutOfRangeChecker should
+    // evaluate.  Checker should either ignore this line or process it without
+    // producing any warnings.  However, compilation will (and should) still
+    // generate a warning having nothing to do with this checker.
     unused; // expected-warning {{expression result unused}}
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,10 +91,21 @@
 
 void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE,
                                              CheckerContext &C) const {
-  // If cast is implicit LValueToRValue, no conversion is taking place,
-  // and therefore no range check is needed.  Don't analyze further.
-  if (CE->getCastKind() == CK_LValueToRValue)
-    return;
+
+  // Only perform enum range check on casts where such checks are valid.  For
+  // all other cast kinds (where enum range checks are unnecessary or invalid),
+  // just return immediately.  TODO: The set of casts whitelisted for enum
+  // range checking may be incomplete.  Better to add a missing cast kind to
+  // enable a missing check than to generate false negatives and have to remove
+  // those later.
+  switch (CE->getCastKind()) {
+    case CK_IntegralCast:
+      break;
+
+    default:
+      return;
+      break;
+  }
 
   // Get the value of the expression to cast.
   const llvm::Optional<DefinedOrUnknownSVal> ValueToCast =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66014.215433.patch
Type: text/x-patch
Size: 5116 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190815/d5e8fcc2/attachment-0001.bin>