New warning for misuse of builtin compare functions.

[rtrieu]

Summary

This warning catches misuse of builtin compare functions memcmp, strcmp, strncmp, strcasecmp, strncasecmp, and their __builtin_ equivalents. These functions return an int representing the relationship between two arguments. Some coders mistaken {-1, 0, 1} as the only three valid return values when the function can return any int value.

This warning is disabled for one of the static analysis tests since the static analyzer has the three return value assumption built in.

include/clang/Basic/DiagnosticGroups.td

 def GNUAlignofExpression : DiagGroup<"gnu-alignof-expression">;
 def AmbigMemberTemplate : DiagGroup<"ambiguous-member-template">;
 def GNUAnonymousStruct : DiagGroup<"gnu-anonymous-struct">;
 def ArrayBounds : DiagGroup<"array-bounds">;
 def ArrayBoundsPointerArithmetic : DiagGroup<"array-bounds-pointer-arithmetic">;
 def Availability : DiagGroup<"availability">;
 def Section : DiagGroup<"section">;
 def AutoImport : DiagGroup<"auto-import">;
+def BuiltinCompare : DiagGroup<"builtin-compare">;
 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
 def ConstantConversion :
   DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
 def LiteralConversion : DiagGroup<"literal-conversion">;
 def StringConversion : DiagGroup<"string-conversion">;
 def SignConversion : DiagGroup<"sign-conversion">;

include/clang/Basic/DiagnosticSemaKinds.td

def warn_abs_too_small : Warning<
   "absolute value function %0 given an argument of type %1 but has parameter "
   "of type %2 which may cause truncation of value">, InGroup<AbsoluteValue>;
 def warn_wrong_absolute_value_type : Warning<
   "using %select{integer|floating point|complex}1 absolute value function %0 "
   "when argument is of %select{integer|floating point|complex}2 type">,
   InGroup<AbsoluteValue>;
 def note_replace_abs_function : Note<"use function '%0' instead">;
 
+def warn_builtin_compare_comparison : Warning<
+  "builtin compare function '%0' can return any int value, "
+  "not just values -1, 0, or 1">,
+  InGroup<BuiltinCompare>;
+
 def warn_infinite_recursive_function : Warning<
   "all paths through this function will call itself">,
   InGroup<InfiniteRecursion>, DefaultIgnore;
 
 // Constant expressions
 def err_expr_not_ice : Error<
   "expression is not an %select{integer|integral}0 constant expression">;
 def ext_expr_not_ice : Extension<

lib/Sema/SemaChecking.cpp

S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison)
       << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
   } else if (op == BO_LE && IsZero(S, E->getLHS())) {
     S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison)
       << "0 <=" << "true" << HasEnumType(E->getRHS())
       << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
   }
 }
 
-static void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E,
-                                         Expr *Constant, Expr *Other,
+static void DiagnoseOutOfRangeComparison(Sema &S, const BinaryOperator *E,
+                                         const Expr *Constant,
+                                         const Expr *Other,
                                          llvm::APSInt Value,
                                          bool RhsConstant) {
   // Disable warning in template instantiations.
   if (!S.ActiveTemplateInstantiations.empty())
     return;
 
   // TODO: Investigate using GetExprRange() to get tighter bounds
   // on the bit ranges.
static void DiagnoseOutOfRangeComparison(Sema &S, const BinaryOperator *E,
   S.DiagRuntimeBehavior(
     E->getOperatorLoc(), E,
     S.PDiag(diag::warn_out_of_range_compare)
         << OS.str() << LiteralOrBoolConstant
         << OtherT << (OtherIsBooleanType && !OtherT->isBooleanType()) << IsTrue
         << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
 }
 
+/// Check compares of builtin compare functions.
+static void DiagnoseBuiltinCompareFunction(Sema &S, const BinaryOperator *E,
+                                           const Expr *Constant,
+                                           const Expr *Other,
+                                           llvm::APSInt Value,
+                                           bool RhsConstant) {
+  // FIXME: Handle conversion to unsigned values, i.e. strcmp(a, b) > 0u;
+  if (Value.isUnsigned())
+    return;
+
+  if (Value == 0)
+    return;
+
+  const CallExpr *Call = dyn_cast<CallExpr>(Other);
+  if (!Call)
+    return;
+
+  const FunctionDecl *FDecl = Call->getDirectCallee();
+  if (!FDecl)
+    return;
+
+  unsigned BuiltinFunction = FDecl->getBuiltinID();
+  if (BuiltinFunction == 0)
+    return;
+  switch (BuiltinFunction) {
+  default:
+    return;
+  case Builtin::BI__builtin_memcmp:
+  case Builtin::BI__builtin_strcasecmp:
+  case Builtin::BI__builtin_strcmp:
+  case Builtin::BI__builtin_strncasecmp:
+  case Builtin::BI__builtin_strncmp:
+  case Builtin::BImemcmp:
+  case Builtin::BIstrcasecmp:
+  case Builtin::BIstrcmp:
+  case Builtin::BIstrncasecmp:
+  case Builtin::BIstrncmp:
+    break;
+  }
+
+  const BinaryOperator::Opcode Operator = E->getOpcode();
+  switch (Operator) {
+  default:
+    llvm_unreachable("Unexpected BinaryOperator opcode.");
+  case BO_EQ:
+  case BO_NE:
+    break;
+  case BO_GT:
+    if (RhsConstant && (Value == -1)) // cmp() > -1
+      return;
+    if (!RhsConstant && (Value == 1)) // 1 > cmp()
+      return;
+    break;
+  case BO_LT:
+    if (RhsConstant && (Value == 1)) // cmp() < 1
+      return;
+    if (!RhsConstant && (Value == -1)) // -1 < cmp()
+      return;
+    break;
+  case BO_GE:
+    if (RhsConstant && (Value == 1)) // cmp() >= 1
+      return;
+    if (!RhsConstant && (Value == -1)) // -1 >= cmp()
+      return;
+    break;
+  case BO_LE:
+    if (RhsConstant && (Value == -1)) // cmp() <= -1
+      return;
+    if (!RhsConstant && (Value == 1)) // 1 <= cmp()
+      return;
+    break;
+  }
+
+  S.Diag(E->getOperatorLoc(), diag::warn_builtin_compare_comparison)
+      << S.Context.BuiltinInfo.GetName(BuiltinFunction);
+}
+
+/// Perform diagnostics when exactly one argument to a comparison expression
+/// is a constant expression.
+static void DiagnoseConstantComparison(Sema &S, const BinaryOperator *E,
+                                       const Expr *LHS, const Expr *RHS,
+                                       bool IsLHSIntegralLiteral,
+                                       bool IsRHSIntegralLiteral,
+                                       llvm::APSInt LHSValue,
+                                       llvm::APSInt RHSValue) {
+  if (!IsLHSIntegralLiteral && !IsRHSIntegralLiteral)
+    return;
+  if (IsLHSIntegralLiteral && IsRHSIntegralLiteral)
+    return;
+
+  const bool RHSConstant = IsRHSIntegralLiteral;
+  const Expr *ConstantExpr = RHSConstant ? RHS :
+              LHS;
+  const Expr *OtherExpr = RHSConstant ? LHS : RHS;
+  llvm::APSInt Value = RHSConstant ? RHSValue : LHSValue;
+
+  DiagnoseOutOfRangeComparison(S, E, ConstantExpr, OtherExpr, Value,
+                               RHSConstant);
+  DiagnoseBuiltinCompareFunction(S, E, ConstantExpr, OtherExpr, Value,
+                                 RHSConstant);
+}
+
 /// Analyze the operands of the given comparison.  Implements the
 /// fallback case from AnalyzeComparison.
 static void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) {
   AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
 }
 
 /// \brief Implements -Wsign-compare.
if (!S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType()))
     return AnalyzeImpConvsInComparison(S, E);
 
   // Don't analyze value-dependent comparisons directly.
   if (E->isValueDependent())
     return AnalyzeImpConvsInComparison(S, E);
 
   Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
   Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
-  
   bool IsComparisonConstant = false;
-  
   // Check whether an integer constant comparison results in a value
   // of 'true' or 'false'.
   if (T->isIntegralType(S.Context)) {
-    llvm::APSInt RHSValue;
-    bool IsRHSIntegralLiteral = 
-      RHS->isIntegerConstantExpr(RHSValue, S.Context);
-    llvm::APSInt LHSValue;
-    bool IsLHSIntegralLiteral = 
+    llvm::APSInt LHSValue, RHSValue;
+    const bool IsLHSIntegralLiteral =
         LHS->isIntegerConstantExpr(LHSValue, S.Context);
-    if (IsRHSIntegralLiteral && !IsLHSIntegralLiteral)
-        DiagnoseOutOfRangeComparison(S, E, RHS, LHS, RHSValue, true);
-    else if (!IsRHSIntegralLiteral && IsLHSIntegralLiteral)
-      DiagnoseOutOfRangeComparison(S, E, LHS, RHS, LHSValue, false);
-    else
-      IsComparisonConstant = 
-        (IsRHSIntegralLiteral && IsLHSIntegralLiteral);
+    const bool IsRHSIntegralLiteral =
+        RHS->isIntegerConstantExpr(RHSValue, S.Context);
+    DiagnoseConstantComparison(S, E, LHS, RHS, IsLHSIntegralLiteral,
+                               IsRHSIntegralLiteral, LHSValue, RHSValue);
+    IsComparisonConstant = IsRHSIntegralLiteral && IsLHSIntegralLiteral;
   } else if (!T->hasUnsignedIntegerRepresentation())
-      IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
-  
   // We don't do anything special if this isn't an unsigned integral
   // comparison:  we're only interested in integral comparisons, and
   // signed comparisons only happen in cases we don't care to warn about.
   //
   // We also don't care about value-dependent expressions or expressions
   // whose result is a constant.
   if (!T->hasUnsignedIntegerRepresentation() || IsComparisonConstant)
     return AnalyzeImpConvsInComparison(S, E);
-  
   // Check to see if one of the (unmodified) operands is of different
   // signedness.
   Expr *signedOperand, *unsignedOperand;
   if (LHS->getType()->hasSignedIntegerRepresentation()) {
     assert(!RHS->getType()->hasSignedIntegerRepresentation() &&
            "unsigned comparison between two signed integer expressions?");
     signedOperand = LHS;
     unsignedOperand = RHS;

test/Analysis/string.c

-// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -Wno-builtin-compare -verify %s
+// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -Wno-builtin-compare -verify %s
+// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -Wno-builtin-compare -verify %s
+// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -Wno-builtin-compare -verify %s
 
 //===----------------------------------------------------------------------===
 // Declarations
 //===----------------------------------------------------------------------===
 
 // Some functions are so similar to each other that they follow the same code
 // path, such as memcpy and __memcpy_chk, or memcmp and bcmp. If VARIANT is
 // defined, make sure to use the variants instead to make sure they are still

test/Sema/warn-builtin-compare.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef __SIZE_TYPE__ size_t;
+
+int strcmp(const char*, const char*);
+int strncmp(const char*, const char*, size_t);
+int strcasecmp(const char*, const char*);
+int strncasecmp(const char*, const char*, size_t);
+int memcmp(const void*, const void*, size_t);
+
+const char string1[] = "foo";
+const char string2[] = "bar";
+
+void test0() {
+  if (strcmp(string1, string2) == 4) {}
+  // expected-warning@-1{{builtin compare function 'strcmp' can return any int value, not just values -1, 0, or 1}}
+}
+
+// No warning compares with 0.
+void test1() {
+  if (strcmp(string1, string2) > 0) {}
+  if (strcmp(string1, string2) < 0) {}
+  if (strcmp(string1, string2) >= 0) {}
+  if (strcmp(string1, string2) <= 0) {}
+  if (strcmp(string1, string2) == 0) {}
+  if (strcmp(string1, string2) != 0) {}
+
+  if (0 > strcmp(string1, string2)) {}
+  if (0 < strcmp(string1, string2)) {}
+  if (0 >= strcmp(string1, string2)) {}
+  if (0 <= strcmp(string1, string2)) {}
+  if (0 == strcmp(string1, string2)) {}
+  if (0 != strcmp(string1, string2)) {}
+}
+
+// No warning compares with 1 and -1.
+void test2() {
+  if (strcmp(string1, string2) > -1) {}
+  if (strcmp(string1, string2) < 1) {}
+  if (strcmp(string1, string2) >= 1) {}
+  if (strcmp(string1, string2) <= -1) {}
+
+  if (1 > strcmp(string1, string2)) {}
+  if (-1 < strcmp(string1, string2)) {}
+  if (-1 >= strcmp(string1, string2)) {}
+  if (1 <= strcmp(string1, string2)) {}
+}
+
+// Warning with 1 and -1.
+void test3() {
+  if (strcmp(string1, string2) > 1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) < -1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) >= -1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) <= 1) {}  // expected-warning{{}}
+
+  if (-1 > strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (1 < strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (1 >= strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (-1 <= strcmp(string1, string2)) {}  // expected-warning{{}}
+
+  if (strcmp(string1, string2) == 1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) != 1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) == -1) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) != -1) {}  // expected-warning{{}}
+
+  if (1 == strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (-1 == strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (1 != strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (-1 != strcmp(string1, string2)) {}  // expected-warning{{}}
+}
+
+// Warning with other constant.
+void test4() {
+  if (strcmp(string1, string2) > 42) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) < 42) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) >= 42) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) <= 42) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) == 42) {}  // expected-warning{{}}
+  if (strcmp(string1, string2) != 42) {}  // expected-warning{{}}
+
+  if (42 > strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (42 < strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (42 >= strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (42 <= strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (42 == strcmp(string1, string2)) {}  // expected-warning{{}}
+  if (42 != strcmp(string1, string2)) {}  // expected-warning{{}}
+}
+
+// Warning with each function.
+void test5() {
+  if (strcmp(string1, string2) == 1) {}
+  // expected-warning@-1{{'strcmp'}}
+  if (strncmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'strncmp'}}
+  if (strcasecmp(string1, string2) == 1) {}
+  // expected-warning@-1{{'strcasecmp'}}
+  if (strncasecmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'strncasecmp'}}
+  if (memcmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'memcmp'}}
+
+  if (__builtin_strcmp(string1, string2) == 1) {}
+  // expected-warning@-1{{'__builtin_strcmp'}}
+  if (__builtin_strncmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'__builtin_strncmp'}}
+  if (__builtin_strcasecmp(string1, string2) == 1) {}
+  // expected-warning@-1{{'__builtin_strcasecmp'}}
+  if (__builtin_strncasecmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'__builtin_strncasecmp'}}
+  if (__builtin_memcmp(string1, string2, 3) == 1) {}
+  // expected-warning@-1{{'__builtin_memcmp'}}
+}