[PATCH] D20811: [analyzer] Model some library functions

[Artem Dergachev via cfe-commits]

NoQ added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
@@ +190,3 @@
+    // impossible to verify this precisely, but at least
+    // this check avoids potential crashes.
+    bool matchesCall(const CallExpr *CE) const;
----------------
zaks.anna wrote:
> Do we need to talk about crashes when describing what this does?
> Also, please, use oxygen throughout.
Added more comments below.

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:205
@@ +204,3 @@
+  }
+  static QualType getArgType(const CallEvent &Call, ArgNoTy ArgNo) {
+    return ArgNo == Ret ? Call.getResultType().getCanonicalType()
----------------
zaks.anna wrote:
> We could either provide these APIs in CallEvent or at least have variants that return canonical types. Maybe we already do some of that?
Maybe a separate commit? There are quite a few checkers from which the `.getArgExpr(N)->getType()` pattern could be de-duplicated, but i don't think many of them are interested in canonical types.

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:445
@@ +444,3 @@
+
+  const FunctionSpecTy &Spec = FSMI->second;
+  if (!Spec.matchesCall(CE))
----------------
zaks.anna wrote:
> When can this go wrong? Are we checking if there is a mismatch between the function declaration and the call expression? It is strange that findFunctionSpec takes both of those. Couldn't you always get FunctionDecl out of CallExpr?
Callee decl is path-sensitive information because functions can be passed around by pointers, as mentioned in the comment at the beginning of the function. Expanded the comment, added a test.

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:508
@@ +507,3 @@
+  FunctionSpecMap = {
+    // The isascii() family of functions.
+    { "isalnum",
----------------
zaks.anna wrote:
> you could also use /*NameOfTheField*/ convention to name the arguments if that would make this map more readable.
I think compactness is worth it here, and specs are pretty easy to remember, imho. Added an example to the first spec to see how it looks and make it easier for the reader to adapt and remember, but i'm not quite convinced that verbosity is worth it here.

================
Comment at: test/Analysis/std-library-functions.c:3
@@ +2,3 @@
+
+void clang_analyzer_eval(int);
+int glob;
----------------
zaks.anna wrote:
> Why are you not testing all of the functions?
I was too bored to generate tests for all branches of all functions (and if i auto-generate such tests, it defeats the purpose), but i added some of the more creative tests and covered at least some branches of all functions with them.


https://reviews.llvm.org/D20811