[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

The high level idea and the implementation of the checker seems great. In general, things that you want to address in later patches should be stated in the code with a `TODO`. I wrote a couple nits that I don't want to delete, but maybe it'd be better to address them after the dependency patch is agreed upon.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,
----------------
How about we add an example as well?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+    ValueRange negate() const {
+      ValueRange tmp(*this);
----------------
Maybe `complement` would be a better name? That sounds a lot more like a set operation. Also, this function highlights well that inheritance might not be the best solution here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
+  ///   * a list of branches - a list of list of ranges -
+  ///     i.e. a list of lists of lists of segments,
+  ///   * a list of argument constraints, that must be true on every branch.
----------------
I think that is a rather poor example to help understand what `list of list of ranges` means :) -- Could you try to find something better?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:10
 // This checker improves modeling of a few simple library functions.
 // It does not generate warnings.
 //
----------------
I suspect this comment is no longer relevant.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:399-407
+  auto Report = [&](ExplodedNode *N) {
+    if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
+      return;
+    // FIXME Add detailed diagnostic.
+    StringRef Msg = "Function argument constraint is not satisfied";
+    auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+    bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
----------------
While I find your usage of lambdas fascinating, this one seems a bit unnecessary :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:402
+      return;
+    // FIXME Add detailed diagnostic.
+    StringRef Msg = "Function argument constraint is not satisfied";
----------------
That is a `TODO`, rather :^)


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
----------------
Hmm, why do we have 2 different test files that essentially do the same? Shouldn't we only have a single one with `analyzer-output=text`?


================
Comment at: clang/test/Analysis/std-c-library-functions.c:1-31
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
----------------
What a beautiful sight. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73898/new/

https://reviews.llvm.org/D73898