NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin, a.sidorin, xazax.hun.
NoQ added a subscriber: cfe-commits.
The mechanism for filtering out wrong functions with the same name was too
aggressive to filter out eg. `int` vs. `long`, when sizes of both are equal.
Such issues were detected by several buildbots.
In fact, our function summaries are not precise enough to deal with such
differences, and they do not need to be. This patch fixes the issue by only
taking size and signedness into account. Also minor cleanups.
https://reviews.llvm.org/D25940
Files:
lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
test/Analysis/std-c-library-functions.c
Index: test/Analysis/std-c-library-functions.c
===================================================================
--- test/Analysis/std-c-library-functions.c
+++ test/Analysis/std-c-library-functions.c
@@ -1,4 +1,8 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple i686-unknown-linux -analyze -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
// RUN: %clang_cc1 -triple x86_64-unknown-linux -analyze -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple armv7-a15-linux -analyze -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple thumbv7-a15-linux -analyze -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
void clang_analyzer_eval(int);
@@ -22,7 +26,6 @@
clang_analyzer_eval(fgetc(fp) >= 0); // expected-warning{{UNKNOWN}}
}
-
typedef unsigned long size_t;
typedef signed long ssize_t;
ssize_t read(int, void *, size_t);
Index: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -79,11 +79,13 @@
/// impose a constraint that involves other argument or return value symbols.
enum ValueRangeKindTy { OutOfRange, WithinRange, ComparesToArgument };
+ typedef uint64_t RangeIntTy;
+
/// Normally, describes a single range constraint, eg. {{0, 1}, {3, 4}} is
/// a non-negative integer, which less than 5 and not equal to 2. For
/// `ComparesToArgument', holds information about how exactly to compare to
/// the argument.
- typedef std::vector<std::pair<uint64_t, uint64_t>> IntRangeVectorTy;
+ typedef std::vector<std::pair<RangeIntTy, RangeIntTy>> IntRangeVectorTy;
/// A reference to an argument or return value by its number.
/// ArgNo in CallExpr and CallEvent is defined as Unsigned, but
@@ -185,9 +187,11 @@
return T;
}
+ static bool typesAreEqual(QualType Lhs, QualType Rhs, ASTContext &ACtx);
+
/// Try our best to figure out if the call expression is the call of
/// *the* library function to which this specification applies.
- bool matchesCall(const CallExpr *CE) const;
+ bool matchesCall(const CallExpr *CE, ASTContext &ACtx) const;
};
// The map of all functions supported by the checker. It is initialized
@@ -386,31 +390,53 @@
llvm_unreachable("Unknown invalidation kind!");
}
+bool StdLibraryFunctionsChecker::FunctionSummaryTy::typesAreEqual(
+ QualType Lhs, QualType Rhs, ASTContext &ACtx) {
+ // Lhs comes from the FunctionSummary, Rhs comes from the call expression.
+ // Hence lack of symmetry.
+ if (Lhs.isNull()) // "Irrelevant" types.
+ return true;
+
+ assertTypeSuitableForSummary(Lhs);
+ assert(!Rhs.isNull());
+ assert(Lhs.isCanonical());
+ Rhs = Rhs.getCanonicalType();
+
+ // We're only requiring integral types to match by size and signedness,
+ // not be completely identical. Currently the only reason for that is
+ // our inability to guess ssize_t correctly from the AST context.
+ // FIXME: Maybe the proper solution would be to scan through typedefs.
+ if (!Lhs->isIntegralOrEnumerationType())
+ return Lhs == Rhs;
+ if (!Rhs->isIntegralOrEnumerationType())
+ return false;
+
+ if (Lhs->isSignedIntegerOrEnumerationType() !=
+ Rhs->isSignedIntegerOrEnumerationType())
+ return false;
+
+ return ACtx.getTypeSize(Lhs) == ACtx.getTypeSize(Rhs);
+}
+
bool StdLibraryFunctionsChecker::FunctionSummaryTy::matchesCall(
- const CallExpr *CE) const {
+ const CallExpr *CE, ASTContext &ACtx) const {
// Check number of arguments:
if (CE->getNumArgs() != ArgTypes.size())
return false;
// Check return type if relevant:
- if (!RetType.isNull() && RetType != CE->getType().getCanonicalType())
+ if (!typesAreEqual(RetType, CE->getType().getCanonicalType(), ACtx))
return false;
// Check argument types when relevant:
for (size_t I = 0, E = ArgTypes.size(); I != E; ++I) {
QualType FormalT = ArgTypes[I];
// Null type marks irrelevant arguments.
- if (FormalT.isNull())
- continue;
-
- assertTypeSuitableForSummary(FormalT);
QualType ActualT = StdLibraryFunctionsChecker::getArgType(CE, I);
- assert(ActualT.isCanonical());
- if (ActualT != FormalT)
+ if (!typesAreEqual(FormalT, ActualT, ACtx))
return false;
}
-
return true;
}
@@ -443,7 +469,7 @@
// very integral-type-sensitive operations on arguments and
// return values.
const FunctionSummaryTy &Spec = FSMI->second;
- if (!Spec.matchesCall(CE))
+ if (!Spec.matchesCall(CE, C.getASTContext()))
return None;
return Spec;
@@ -464,9 +490,9 @@
QualType SSizeTy = ACtx.getIntTypeForBitwidth(ACtx.getTypeSize(SizeTy), true);
// Don't worry about truncation here, it'd be cast back to SIZE_MAX when used.
- LLVM_ATTRIBUTE_UNUSED int64_t SizeMax =
+ LLVM_ATTRIBUTE_UNUSED RangeIntTy SizeMax =
BVF.getMaxValue(SizeTy).getLimitedValue();
- int64_t SSizeMax =
+ RangeIntTy SSizeMax =
BVF.getMaxValue(SSizeTy).getLimitedValue();
// We are finally ready to define specifications for all supported functions.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
