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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

EnumCastOutOfRangeChecker should not perform enum range checks on LValueToRValue casts, since this type of cast does not actually change the underlying type.   Performing the unnecessary check actually triggered an assertion failure deeper in EnumCastOutOfRange for certain input (which is captured in the accompanying test code).


Repository:
  rC Clang

https://reviews.llvm.org/D66014

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


Index: clang/test/Analysis/enum-cast-out-of-range.cpp
===================================================================
--- clang/test/Analysis/enum-cast-out-of-range.cpp
+++ clang/test/Analysis/enum-cast-out-of-range.cpp
@@ -150,7 +150,15 @@
   scoped_specified_t InvalidAfterRangeEnd = (scoped_specified_t)(5); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
 }
 
-void rangeContstrained1(int input) {
+unscoped_unspecified_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.
+    unused; // expected-warning {{expression result unused}}
+}
+
+void rangeConstrained1(int input) {
   if (input > -5 && input < 5)
     auto value = static_cast<scoped_specified_t>(input); // OK. Being conservative, this is a possibly good value.
 }
Index: clang/test/Analysis/enum-cast-out-of-range.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/enum-cast-out-of-range.c
@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 \
+// 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
+};
+
+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 unscoped_unspecified_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.
+    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,6 +91,11 @@
 
 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;
+
   // Get the value of the expression to cast.
   const llvm::Optional<DefinedOrUnknownSVal> ValueToCast =
       C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66014.214379.patch
Type: text/x-patch
Size: 4553 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190809/58cc486a/attachment-0001.bin>