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