Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -29,6 +29,7 @@ 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">; Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -68,6 +68,11 @@ InGroup; 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; + def warn_infinite_recursive_function : Warning< "all paths through this function will call itself">, InGroup, DefaultIgnore; Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -6089,8 +6089,9 @@ } } -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. @@ -6298,6 +6299,108 @@ << 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(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) { @@ -6323,28 +6426,23 @@ 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 = - 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); + llvm::APSInt LHSValue, RHSValue; + const bool IsLHSIntegralLiteral = + LHS->isIntegerConstantExpr(LHSValue, S.Context); + 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); - + 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. @@ -6353,7 +6451,7 @@ // 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; Index: test/Analysis/string.c =================================================================== --- test/Analysis/string.c +++ test/Analysis/string.c @@ -1,7 +1,7 @@ -// 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 Index: test/Sema/warn-builtin-compare.c =================================================================== --- test/Sema/warn-builtin-compare.c +++ test/Sema/warn-builtin-compare.c @@ -0,0 +1,112 @@ +// 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'}} +}