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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 16 inline comments as done.
martong added inline comments.
Herald added a subscriber: DenisDvlp.


================
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]>,
----------------
Szelethus wrote:
> martong wrote:
> > Szelethus wrote:
> > > How about we add an example as well?
> > You mean like NonNull or other constraints?
> Like
> ```
> Check constraints of arguments of C standard library functions, such as whether the parameter of isalpha is in the range [0, 255] or is EOF.
> ```
Ok, done.


================
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.
----------------
martong wrote:
> Szelethus wrote:
> > 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?
> Yeah, that part definitely should be reworded.
I added an example with `isalpha`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
   class ValueConstraint {
----------------
Szelethus wrote:
> How about `ValueConstraintRef`?
Yeah, we have `ProgramStateRef` and `SymbolRef`. And both are actually just synonyms to smart pointers. I'd rather not call a pointer as a reference, because that can be confusing when reading the code. E.g. when I see that we return with a `nullptr` from a function that can return with a `...Ref` I start to scratch my head.


================
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);
----------------
Szelethus wrote:
> While I find your usage of lambdas fascinating, this one seems a bit unnecessary :)
Ok I moved it to be a member function named `ReportBug`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409
 
-  Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+  for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+    ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);
----------------
NoQ wrote:
> Maybe we should add an assertion that the same argument isn't specified multiple times.
I think there could be cases when we want to have e.g. a not-null constraint on the 1st argument, but also we want to express that the 1st argument's size is described by the 2nd argument. I am planning to implement such a constraints in the future. In that case we would have two constraints on the 1st argument and the assert would fire.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898