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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
