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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 11 inline comments as done.
martong added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:298
 
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions, "
----------------
Szelethus wrote:
> Just noticed, this checker still lies in the `apiModeling` package. Could we find a more appropriate place?
Technically speaking this is still api modeling. In midterm we'd like to add support for more libc functions, gnu and posix functions, they are all library functions i.e. they provide some api.
Of course in long term, we'd like to experiment by getting some constraints from IR/Attributor, but we are still far from there.


================
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.
----------------
balazske wrote:
> martong wrote:
> > 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`.
> The "branches" are the structures that define relations between arguments and return values? This could be included in the description.
Not exactly. A branch represents a path in the exploded graph of a function (which is a tree).
So, a branch is a series of assumptions. In other words, branches represent split states and additional assumptions on top of the splitting assumption. I added this explanation to the comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:112
                                   const Summary &Summary) const = 0;
+    virtual ValueConstraintPtr negate() const {
+      llvm_unreachable("Not implemented");
----------------
balazske wrote:
> Is it better done with `= 0`?
Not all of the constraint classes must implement this. Right now, e.g. the `ComparisonConstraint` does not implement this, because there is no such summary (yet) that uses a `ComparisonConstraint` as an argument constraint.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:260
 
+  BugType BT{this, "Unsatisfied argument constraints", categories::LogicError};
+
----------------
Szelethus wrote:
> By passing `this`, the error message will be tied to the modeling checker, not to the one you just added. `BugType` has a constructor that accepts a string instead, pass `CheckNames[CK_StdCLibraryFunctionArgsChecker]` in there :) Also, how about `BT_InvalidArgument` or something?
Thanks, good catch, I did not know about that. Please note that using `CheckNames` requires that we change the `BT` member to be lazily initialized. Because `CheckNames` is initialized only after the checker itself is created, thus we cannot initialize `BT` during the checkers construction, b/c that would be before `CheckNames` is set. So, I changed `BT` to be a unique_ptr and it is being lazily initialized in `reportBug`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:297
+
+  void ReportBug(const CallEvent &Call, ExplodedNode *N, CheckerContext &C) const {
+    if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
----------------
balazske wrote:
> This should be called `reportBug`.
Yeah, can't get used to this strange naming convention that LLVM uses. Fixed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898