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.
http://reviews.llvm.org/D10374
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/Analysis/string.c
test/Sema/warn-builtin-compare.c
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
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<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;
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<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) {
@@ -6323,37 +6426,32 @@
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.
//
// 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;
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'}}
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
