[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa787a4ed16d6: [analyzer][StdLibraryFunctionsChecker] Use
Optionals throughout the summary API (authored by martong).
Changed prior to commit:
https://reviews.llvm.org/D86531?vs=289099&id=289108#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
Index: clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty
+
+// We test here the implementation of our summary API with Optional types. In
+// this TU we do not provide declaration for any of the functions that have
+// summaries. The implementation should be able to handle the nonexistent
+// declarations in a way that the summary is not added to the map. We expect no
+// crashes (i.e. no optionals should be 'dereferenced') and no output.
+
+// Must have at least one call expression to initialize the summary map.
+int bar(void);
+void foo() {
+ bar();
+}
+
+// CHECK-NOT: Loaded summary for:
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -138,16 +138,24 @@
/// Given a range, should the argument stay inside or outside this range?
enum RangeKind { OutOfRange, WithinRange };
- /// Encapsulates a single range on a single symbol within a branch.
+ /// Encapsulates a range on a single symbol.
class RangeConstraint : public ValueConstraint {
-RangeKind Kind; // Kind of range definition.
-IntRangeVector Args; // Polymorphic arguments.
+RangeKind Kind;
+// A range is formed as a set of intervals (sub-ranges).
+// E.g. {['A', 'Z'], ['a', 'z']}
+//
+// The default constructed RangeConstraint has an empty range set, applying
+// such constraint does not involve any assumptions, thus the State remains
+// unchanged. This is meaningful, if the range is dependent on a looked up
+// type (e.g. [0, Socklen_tMax]). If the type is not found, then the range
+// is default initialized to be empty.
+IntRangeVector Ranges;
public:
-RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Args)
-: ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
+RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Ranges)
+: ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {}
-const IntRangeVector &getRanges() const { return Args; }
+const IntRangeVector &getRanges() const { return Ranges; }
private:
ProgramStateRef applyAsOutOfRange(ProgramStateRef State,
@@ -313,7 +321,8 @@
/// The complete list of constraints that defines a single branch.
typedef std::vector ConstraintSet;
- using ArgTypes = std::vector;
+ using ArgTypes = std::vector>;
+ using RetType = Optional;
// A placeholder type, we use it whenever we do not care about the concrete
// type in a Signature.
@@ -323,17 +332,37 @@
// The signature of a function we want to describe with a summary. This is a
// concessive signature, meaning there may be irrelevant types in the
// signature which we do not check against a function with concrete types.
- struct Signature {
-ArgTypes ArgTys;
+ // All types in the spec need to be canonical.
+ class Signature {
+using ArgQualTypes = std::vector;
+ArgQualTypes ArgTys;
QualType RetTy;
-Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
- assertRetTypeSuitableForSignature(RetTy);
- for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
-QualType ArgTy = ArgTys[I];
-assertArgTypeSuitableForSignature(ArgTy);
+// True if any component type is not found by lookup.
+bool Invalid = false;
+
+ public:
+// Construct a signature from optional types. If any of the optional types
+// are not set then the signature will be invalid.
+Signature(ArgTypes ArgTys, RetType RetTy) {
+ for (Optional Arg : ArgTys) {
+if (!
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
balazske accepted this revision. balazske added a comment. This revision is now accepted and ready to land. Looks good now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86531/new/ https://reviews.llvm.org/D86531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong updated this revision to Diff 289099.
martong added a comment.
- Support empty ranges
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
Index: clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty
+
+// We test here the implementation of our summary API with Optional types. In
+// this TU we do not provide declaration for any of the functions that have
+// summaries. The implementation should be able to handle the nonexistent
+// declarations in a way that the summary is not added to the map. We expect no
+// crashes (i.e. no optionals should be 'dereferenced') and no output.
+
+// Must have at least one call expression to initialize the summary map.
+int bar(void);
+void foo() {
+ bar();
+}
+
+// CHECK-NOT: Loaded summary for:
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -138,18 +138,24 @@
/// Given a range, should the argument stay inside or outside this range?
enum RangeKind { OutOfRange, WithinRange };
- /// Encapsulates a single range on a single symbol within a branch.
+ /// Encapsulates a range on a single symbol.
class RangeConstraint : public ValueConstraint {
-RangeKind Kind; // Kind of range definition.
-IntRangeVector Args; // Polymorphic arguments.
+RangeKind Kind;
+// A range is formed as a set of intervals (sub-ranges).
+// E.g. {['A', 'Z'], ['a', 'z']}
+//
+// The default constructed RangeConstraint has an empty range set, applying
+// such constraint does not involve any assumptions, thus the State remains
+// unchanged. This is meaningful, if the range is dependent on a looked up
+// type (e.g. [0, Socklen_tMax]). If the type is not found, then the range
+// is default initialized to be empty.
+IntRangeVector Ranges;
public:
-RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Args)
-: ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
+RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Ranges)
+: ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {}
-const IntRangeVector &getRanges() const {
- return Args;
-}
+const IntRangeVector &getRanges() const { return Ranges; }
private:
ProgramStateRef applyAsOutOfRange(ProgramStateRef State,
@@ -314,7 +320,8 @@
/// The complete list of constraints that defines a single branch.
typedef std::vector ConstraintSet;
- using ArgTypes = std::vector;
+ using ArgTypes = std::vector>;
+ using RetType = Optional;
// A placeholder type, we use it whenever we do not care about the concrete
// type in a Signature.
@@ -324,17 +331,37 @@
// The signature of a function we want to describe with a summary. This is a
// concessive signature, meaning there may be irrelevant types in the
// signature which we do not check against a function with concrete types.
- struct Signature {
-ArgTypes ArgTys;
+ // All types in the spec need to be canonical.
+ class Signature {
+using ArgQualTypes = std::vector;
+ArgQualTypes ArgTys;
QualType RetTy;
-Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
- assertRetTypeSuitableForSignature(RetTy);
- for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
-QualType ArgTy = ArgTys[I];
-assertArgTypeSuitableForSignature(ArgTy);
+// True if any component type is not found by lookup.
+bool Invalid = false;
+
+ public:
+// Construct a signature from optional types. If any of the optional types
+// are not set then the signature will be invalid.
+Signature(ArgTypes ArgTys, RetType RetTy) {
+ for (Optional Arg : ArgTys) {
+if (!Arg) {
+ Invalid = true;
+ return;
+} else {
+ assertArgTypeSuitableForSignature(*Arg);
+ this->ArgTys.push_back(*Arg);
+}
+ }
+ if (!RetTy) {
+Invalid = true;
+retu
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong updated this revision to Diff 288958.
martong added a comment.
- Rebase to correct base
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
Index: clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty
+
+// We test here the implementation of our summary API with Optional types. In
+// this TU we do not provide declaration for any of the functions that have
+// summaries. The implementation should be able to handle the nonexistent
+// declarations in a way that the summary is not added to the map. We expect no
+// crashes (i.e. no optionals should be 'dereferenced') and no output.
+
+// Must have at least one call expression to initialize the summary map.
+int bar(void);
+void foo() {
+ bar();
+}
+
+// CHECK-NOT: Loaded summary for:
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -314,7 +314,8 @@
/// The complete list of constraints that defines a single branch.
typedef std::vector ConstraintSet;
- using ArgTypes = std::vector;
+ using ArgTypes = std::vector>;
+ using RetType = Optional;
// A placeholder type, we use it whenever we do not care about the concrete
// type in a Signature.
@@ -324,17 +325,37 @@
// The signature of a function we want to describe with a summary. This is a
// concessive signature, meaning there may be irrelevant types in the
// signature which we do not check against a function with concrete types.
- struct Signature {
-ArgTypes ArgTys;
+ // All types in the spec need to be canonical.
+ class Signature {
+using ArgQualTypes = std::vector;
+ArgQualTypes ArgTys;
QualType RetTy;
-Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
- assertRetTypeSuitableForSignature(RetTy);
- for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
-QualType ArgTy = ArgTys[I];
-assertArgTypeSuitableForSignature(ArgTy);
+// True if any component type is not found by lookup.
+bool Invalid = false;
+
+ public:
+// Construct a signature from optional types. If any of the optional types
+// are not set then the signature will be invalid.
+Signature(ArgTypes ArgTys, RetType RetTy) {
+ for (Optional Arg : ArgTys) {
+if (!Arg) {
+ Invalid = true;
+ return;
+} else {
+ assertArgTypeSuitableForSignature(*Arg);
+ this->ArgTys.push_back(*Arg);
+}
+ }
+ if (!RetTy) {
+Invalid = true;
+return;
+ } else {
+assertRetTypeSuitableForSignature(*RetTy);
+this->RetTy = *RetTy;
}
}
+bool isInvalid() const { return Invalid; }
bool matches(const FunctionDecl *FD) const;
private:
@@ -388,6 +409,9 @@
/// rules for the given parameter's type, those rules are checked once the
/// signature is matched.
class Summary {
+// FIXME Probably the Signature should not be part of the Summary,
+// We can remove once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
Optional Sign;
const InvalidationKind InvalidationKd;
Cases CaseConstraints;
@@ -398,11 +422,13 @@
const FunctionDecl *FD = nullptr;
public:
-Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
+Summary(ArgTypes ArgTys, RetType RetTy, InvalidationKind InvalidationKd)
: Sign(Signature(ArgTys, RetTy)), InvalidationKd(InvalidationKd) {}
Summary(InvalidationKind InvalidationKd) : InvalidationKd(InvalidationKd) {}
+// FIXME Remove, once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
Summary &setSignature(const Signature &S) {
Sign = S;
return *this;
@@ -438,6 +464,13 @@
return Result;
}
+// FIXME Remove, once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
+bool hasInvalidSignature
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong updated this revision to Diff 288957.
martong added a comment.
- Revert "Add assert in getRanges"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
clang/test/Analysis/std-c-library-functions-POSIX-socket-sockaddr.cpp
clang/test/Analysis/std-c-library-functions-POSIX.c
Index: clang/test/Analysis/std-c-library-functions-POSIX.c
===
--- clang/test/Analysis/std-c-library-functions-POSIX.c
+++ clang/test/Analysis/std-c-library-functions-POSIX.c
@@ -79,6 +79,22 @@
// CHECK: Loaded summary for: int execv(const char *path, char *const argv[])
// CHECK: Loaded summary for: int execvp(const char *file, char *const argv[])
// CHECK: Loaded summary for: int getopt(int argc, char *const argv[], const char *optstring)
+// CHECK: Loaded summary for: int accept(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len)
+// CHECK: Loaded summary for: int bind(int socket, __CONST_SOCKADDR_ARG address, socklen_t address_len)
+// CHECK: Loaded summary for: int getpeername(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len)
+// CHECK: Loaded summary for: int getsockname(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len)
+// CHECK: Loaded summary for: int connect(int socket, __CONST_SOCKADDR_ARG address, socklen_t address_len)
+// CHECK: Loaded summary for: ssize_t recvfrom(int socket, void *restrict buffer, size_t length, int flags, __SOCKADDR_ARG address, socklen_t *restrict address_len)
+// CHECK: Loaded summary for: ssize_t sendto(int socket, const void *message, size_t length, int flags, __CONST_SOCKADDR_ARG dest_addr, socklen_t dest_len)
+// CHECK: Loaded summary for: int listen(int sockfd, int backlog)
+// CHECK: Loaded summary for: ssize_t recv(int sockfd, void *buf, size_t len, int flags)
+// CHECK: Loaded summary for: ssize_t recvmsg(int sockfd, struct msghdr *msg, int flags)
+// CHECK: Loaded summary for: ssize_t sendmsg(int sockfd, const struct msghdr *msg, int flags)
+// CHECK: Loaded summary for: int setsockopt(int socket, int level, int option_name, const void *option_value, socklen_t option_len)
+// CHECK: Loaded summary for: int getsockopt(int socket, int level, int option_name, void *restrict option_value, socklen_t *restrict option_len)
+// CHECK: Loaded summary for: ssize_t send(int sockfd, const void *buf, size_t len, int flags)
+// CHECK: Loaded summary for: int socketpair(int domain, int type, int protocol, int sv[2])
+// CHECK: Loaded summary for: int getnameinfo(const struct sockaddr *restrict sa, socklen_t salen, char *restrict node, socklen_t nodelen, char *restrict service, socklen_t servicelen, int flags)
long a64l(const char *str64);
char *l64a(long value);
@@ -171,6 +187,46 @@
int execvp(const char *file, char *const argv[]);
int getopt(int argc, char *const argv[], const char *optstring);
+// In some libc implementations, sockaddr parameter is a transparent
+// union of the underlying sockaddr_ pointers instead of being a
+// pointer to struct sockaddr.
+// We match that with the joker Irrelevant type.
+struct sockaddr;
+struct sockaddr_at;
+#define __SOCKADDR_ALLTYPES\
+ __SOCKADDR_ONETYPE(sockaddr) \
+ __SOCKADDR_ONETYPE(sockaddr_at)
+#define __SOCKADDR_ONETYPE(type) struct type *__restrict __##type##__;
+typedef union {
+ __SOCKADDR_ALLTYPES
+} __SOCKADDR_ARG __attribute__((__transparent_union__));
+#undef __SOCKADDR_ONETYPE
+#define __SOCKADDR_ONETYPE(type) const struct type *__restrict __##type##__;
+typedef union {
+ __SOCKADDR_ALLTYPES
+} __CONST_SOCKADDR_ARG __attribute__((__transparent_union__));
+#undef __SOCKADDR_ONETYPE
+typedef unsigned socklen_t;
+
+int accept(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len);
+int bind(int socket, __CONST_SOCKADDR_ARG address, socklen_t address_len);
+int getpeername(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len);
+int getsockname(int socket, __SOCKADDR_ARG address, socklen_t *restrict address_len);
+int connect(int socket, __CONST_SOCKADDR_ARG address, socklen_t address_len);
+ssize_t recvfrom(int socket, void *restrict buffer, size_t length, int flags, __SOCKADDR_ARG address, socklen_t *restrict address_len);
+ssize_t sendto(int socket, const void *message, size_t length, int flags, __CONST_SOCKADDR_ARG dest_addr, socklen_t dest_len);
+
+int listen(int sockfd, int backlog);
+ssize_t recv(int sockfd, void *buf, size_t len, int flags);
+struct msghdr;
+ssize_t recvmsg(int sockfd, struct msghdr *msg, int flags);
+ssize_t sendmsg(int sockfd, const struct msghdr *msg, int flags);
+int setsockopt(int socket, int level, int option_name, const void *option_value, socklen_t option_len);
+int getsockopt(int socket, int level, int option_name, void *restri
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong added a comment.
Just realized. There maybe cases when we'd like to give XMax to an arg
constraint, but the type of the arg is Y (X may be a looked up type). One
example for such is getchar, where the return type is Int, but we have a range
constraint {0, UCharRangeMax}. Consequently, it seems to be better to remove
the assert, because we will not be able to handle such cases with looked up
types and their max values.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong added inline comments.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1006
+return IntRangeVector{std::pair{b, *e}};
+ return IntRangeVector{};
+}
balazske wrote:
> This return of empty vector and possibility of adding empty vector to range
> constraint is a new thing, probably it is better to check at create of range
> constraint (or other place) for empty range (in this case the summary could
> be made invalid)? But this occurs probably never because the matching type
> (of the max value) should be used at the same function and if the type is
> there the max value should be too.
Alright, I added an assert to RangeConstraint::getRanges:
```
const IntRangeVector &getRanges() const {
+ // When using max values for a type, the type normally should be part of
+ // the signature. Thus we must had looked up previously the type. If the
+ // type is not found then the range would be empty, but then the summary
+ // should be invalid too.
+ assert(Args.size() && "Empty range is meaningless");
return Args;
}
```
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
-Optional Mode_tTy = lookupType("mode_t", ACtx);
+Optional Mode_tTy = lookupTy("mode_t");
balazske wrote:
> martong wrote:
> > balazske wrote:
> > > It is better to get every type at the start before adding functions, or
> > > group the functions and get some types at the start of these groups but
> > > mark the groups at least with comments.
> > Well, with looked-up types I followed the usual convention to define a
> > variable right before using it. This means that we lookup a type just
> > before we try to add the function which first uses that type.
> >
> > However, builtin types are defined at the beginning, because they are used
> > very often.
> I still like it better if all the type variables are created at one place
> (can be more easy to maintain if order changes and we have one block of types
> and one of functions) but this is no reason to block this change.
I see your point, still I'd keep this way, because I this way the functions and
the types they use are close to each other in the source.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong updated this revision to Diff 288946.
martong added a comment.
- Add assert in getRanges
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
Index: clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty
+
+// We test here the implementation of our summary API with Optional types. In
+// this TU we do not provide declaration for any of the functions that have
+// summaries. The implementation should be able to handle the nonexistent
+// declarations in a way that the summary is not added to the map. We expect no
+// crashes (i.e. no optionals should be 'dereferenced') and no output.
+
+// Must have at least one call expression to initialize the summary map.
+int bar(void);
+void foo() {
+ bar();
+}
+
+// CHECK-NOT: Loaded summary for:
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -148,6 +148,11 @@
: ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
const IntRangeVector &getRanges() const {
+ // When using max values for a type, the type normally should be part of
+ // the signature. Thus we must had looked up previously the type. If the
+ // type is not found then the range would be empty, but then the summary
+ // should be invalid too.
+ assert(Args.size() && "Empty range is meaningless");
return Args;
}
@@ -314,7 +319,8 @@
/// The complete list of constraints that defines a single branch.
typedef std::vector ConstraintSet;
- using ArgTypes = std::vector;
+ using ArgTypes = std::vector>;
+ using RetType = Optional;
// A placeholder type, we use it whenever we do not care about the concrete
// type in a Signature.
@@ -324,17 +330,37 @@
// The signature of a function we want to describe with a summary. This is a
// concessive signature, meaning there may be irrelevant types in the
// signature which we do not check against a function with concrete types.
- struct Signature {
-ArgTypes ArgTys;
+ // All types in the spec need to be canonical.
+ class Signature {
+using ArgQualTypes = std::vector;
+ArgQualTypes ArgTys;
QualType RetTy;
-Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
- assertRetTypeSuitableForSignature(RetTy);
- for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
-QualType ArgTy = ArgTys[I];
-assertArgTypeSuitableForSignature(ArgTy);
+// True if any component type is not found by lookup.
+bool Invalid = false;
+
+ public:
+// Construct a signature from optional types. If any of the optional types
+// are not set then the signature will be invalid.
+Signature(ArgTypes ArgTys, RetType RetTy) {
+ for (Optional Arg : ArgTys) {
+if (!Arg) {
+ Invalid = true;
+ return;
+} else {
+ assertArgTypeSuitableForSignature(*Arg);
+ this->ArgTys.push_back(*Arg);
+}
+ }
+ if (!RetTy) {
+Invalid = true;
+return;
+ } else {
+assertRetTypeSuitableForSignature(*RetTy);
+this->RetTy = *RetTy;
}
}
+bool isInvalid() const { return Invalid; }
bool matches(const FunctionDecl *FD) const;
private:
@@ -388,6 +414,9 @@
/// rules for the given parameter's type, those rules are checked once the
/// signature is matched.
class Summary {
+// FIXME Probably the Signature should not be part of the Summary,
+// We can remove once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
Optional Sign;
const InvalidationKind InvalidationKd;
Cases CaseConstraints;
@@ -398,11 +427,13 @@
const FunctionDecl *FD = nullptr;
public:
-Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
+Summary(ArgTypes ArgTys, RetType RetTy, InvalidationKind InvalidationKd)
: Sign(Signature(ArgTys, RetTy)), InvalidationKd(Invalidation
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
balazske added inline comments.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1006
+return IntRangeVector{std::pair{b, *e}};
+ return IntRangeVector{};
+}
This return of empty vector and possibility of adding empty vector to range
constraint is a new thing, probably it is better to check at create of range
constraint (or other place) for empty range (in this case the summary could be
made invalid)? But this occurs probably never because the matching type (of the
max value) should be used at the same function and if the type is there the max
value should be too.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
-Optional Mode_tTy = lookupType("mode_t", ACtx);
+Optional Mode_tTy = lookupTy("mode_t");
martong wrote:
> balazske wrote:
> > It is better to get every type at the start before adding functions, or
> > group the functions and get some types at the start of these groups but
> > mark the groups at least with comments.
> Well, with looked-up types I followed the usual convention to define a
> variable right before using it. This means that we lookup a type just before
> we try to add the function which first uses that type.
>
> However, builtin types are defined at the beginning, because they are used
> very often.
I still like it better if all the type variables are created at one place (can
be more easy to maintain if order changes and we have one block of types and
one of functions) but this is no reason to block this change.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong marked 3 inline comments as done. martong added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86531/new/ https://reviews.llvm.org/D86531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong marked 3 inline comments as done.
martong added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:851
+ } getPointerTy(ACtx);
+ class {
+ public:
balazske wrote:
> Why has this class different layout than `GetPointerTy` and `GetRestrictTy`
> (beneath that here is no `ASTContext`)? It would be better if all these
> classes look the same way: First is the operator with `QualType`, then
> operator with `Optional` that calls the other version of the operator. And
> all of these can be "unnamed" classes?
I'd had the same thoughts before, and was thinking about that maybe a class
template would suffice for all these, but then I realized the following:
To make a type const, we don't need the ASTContext, that is the reason of the
difference.
However, to make a type restricted or a pointer, we need the ASTContext. This
is a legacy non-symmetry from the AST.
> And all of these can be "unnamed" classes?
GetPointerTy and GetRestrictTy need the ASTContext in their constructor. And
you cannot define a constructor to an unnamed class because you cannot write
down the nonexistent name.
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:890
- const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *
- const QualType IntPtrTy = ACtx.getPointerType(IntTy); // int *
+ const QualType VoidPtrTy = getPointerTy(VoidTy); // void *
+ const QualType IntPtrTy = getPointerTy(IntTy); // int *
balazske wrote:
> Is it better to use `ACtx.VoidPtrTy`?
I'd like to use the `ACtx` as less as possible: just to get the basic types.
And from that on we can use our convenience API (GetConstTy, GetPointerTy, etc)
to define all of the derived types... and we can do that in a declarative form
(like ASTMatchters).
Also, the declarative definition of these types now look the same regardless
the type is "derived" from a looked-up type or from a built-in type.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
-Optional Mode_tTy = lookupType("mode_t", ACtx);
+Optional Mode_tTy = lookupTy("mode_t");
balazske wrote:
> It is better to get every type at the start before adding functions, or group
> the functions and get some types at the start of these groups but mark the
> groups at least with comments.
Well, with looked-up types I followed the usual convention to define a variable
right before using it. This means that we lookup a type just before we try to
add the function which first uses that type.
However, builtin types are defined at the beginning, because they are used very
often.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
balazske added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:851
+ } getPointerTy(ACtx);
+ class {
+ public:
Why has this class different layout than `GetPointerTy` and `GetRestrictTy`
(beneath that here is no `ASTContext`)? It would be better if all these classes
look the same way: First is the operator with `QualType`, then operator with
`Optional` that calls the other version of the operator. And all of these can
be "unnamed" classes?
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:890
- const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *
- const QualType IntPtrTy = ACtx.getPointerType(IntTy); // int *
+ const QualType VoidPtrTy = getPointerTy(VoidTy); // void *
+ const QualType IntPtrTy = getPointerTy(IntTy); // int *
Is it better to use `ACtx.VoidPtrTy`?
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
-Optional Mode_tTy = lookupType("mode_t", ACtx);
+Optional Mode_tTy = lookupTy("mode_t");
It is better to get every type at the start before adding functions, or group
the functions and get some types at the start of these groups but mark the
groups at least with comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API
martong updated this revision to Diff 287680.
martong added a comment.
- Use FileCheck in the lit test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86531/new/
https://reviews.llvm.org/D86531
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
Index: clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-POSIX-lookup.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux 2>&1 | FileCheck %s --allow-empty
+
+// We test here the implementation of our summary API with Optional types. In
+// this TU we do not provide declaration for any of the functions that have
+// summaries. The implementation should be able to handle the nonexistent
+// declarations in a way that the summary is not added to the map. We expect no
+// crashes (i.e. no optionals should be 'dereferenced') and no output.
+
+// Must have at least one call expression to initialize the summary map.
+int bar(void);
+void foo() {
+ bar();
+}
+
+// CHECK-NOT: Loaded summary for:
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -314,7 +314,8 @@
/// The complete list of constraints that defines a single branch.
typedef std::vector ConstraintSet;
- using ArgTypes = std::vector;
+ using ArgTypes = std::vector>;
+ using RetType = Optional;
// A placeholder type, we use it whenever we do not care about the concrete
// type in a Signature.
@@ -324,17 +325,37 @@
// The signature of a function we want to describe with a summary. This is a
// concessive signature, meaning there may be irrelevant types in the
// signature which we do not check against a function with concrete types.
- struct Signature {
-ArgTypes ArgTys;
+ // All types in the spec need to be canonical.
+ class Signature {
+using ArgQualTypes = std::vector;
+ArgQualTypes ArgTys;
QualType RetTy;
-Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
- assertRetTypeSuitableForSignature(RetTy);
- for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
-QualType ArgTy = ArgTys[I];
-assertArgTypeSuitableForSignature(ArgTy);
+// True if any component type is not found by lookup.
+bool Invalid = false;
+
+ public:
+// Construct a signature from optional types. If any of the optional types
+// are not set then the signature will be invalid.
+Signature(ArgTypes ArgTys, RetType RetTy) {
+ for (Optional Arg : ArgTys) {
+if (!Arg) {
+ Invalid = true;
+ return;
+} else {
+ assertArgTypeSuitableForSignature(*Arg);
+ this->ArgTys.push_back(*Arg);
+}
+ }
+ if (!RetTy) {
+Invalid = true;
+return;
+ } else {
+assertRetTypeSuitableForSignature(*RetTy);
+this->RetTy = *RetTy;
}
}
+bool isInvalid() const { return Invalid; }
bool matches(const FunctionDecl *FD) const;
private:
@@ -388,6 +409,9 @@
/// rules for the given parameter's type, those rules are checked once the
/// signature is matched.
class Summary {
+// FIXME Probably the Signature should not be part of the Summary,
+// We can remove once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
Optional Sign;
const InvalidationKind InvalidationKd;
Cases CaseConstraints;
@@ -398,11 +422,13 @@
const FunctionDecl *FD = nullptr;
public:
-Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
+Summary(ArgTypes ArgTys, RetType RetTy, InvalidationKind InvalidationKd)
: Sign(Signature(ArgTys, RetTy)), InvalidationKd(InvalidationKd) {}
Summary(InvalidationKind InvalidationKd) : InvalidationKd(InvalidationKd) {}
+// FIXME Remove, once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
Summary &setSignature(const Signature &S) {
Sign = S;
return *this;
@@ -438,6 +464,13 @@
return Result;
}
+// FIXME Remove, once all overload of addToFunctionSummaryMap requires the
+// Signature explicitly given.
+bool hasInvalidSi
