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

martong added a comment. I just created a quick fix for the issue: https://reviews.llvm.org/D76790 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/ https://reviews.llvm.org/D73898 ___

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

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) { +ProgramStateRef SuccessSt =

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) { +ProgramStateRef SuccessSt =

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

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) { +ProgramStateRef SuccessSt =

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG94061df6e5f2: [analyzer] StdLibraryFunctionsChecker: Add argument constraints (authored by martong). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

martong updated this revision to Diff 251648. martong marked an inline comment as done. martong added a comment. - Use prefixes for -verify to check different things in the same test file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/

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

martong marked an inline comment as done. martong added a comment. Thanks for the review guys! Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7 +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN:

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Whoo! The patch looks great and well thought out, the tests look like they cover everything and we also talked about plans for future patches. Excellent! I left a nit about merging the

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

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

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

martong updated this revision to Diff 251083. martong marked 5 inline comments as done. martong added a comment. - Add comments about what is a branch - Do not use 'this' for BugType - Lazily init BT and BT -> BT_InvalidArg - ReportBug -> reportBug Repository: rG LLVM Github Monorepo CHANGES

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

balazske added inline comments. 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

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

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. LGTM, aside from some checker tagging nightmare. Its a bit easy to mess up, please allow me to have a final look before commiting! :) Comment at:

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

martong updated this revision to Diff 250869. martong marked an inline comment as done. martong added a comment. Herald added a subscriber: ASDenysPetrov. - tmp -> Tmp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/

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

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:152 +break; + default: +llvm_unreachable("Unknown RangeConstraint kind!"); martong wrote: > This `default` branch is not needed here

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

martong updated this revision to Diff 250820. martong marked 4 inline comments as done. martong removed a subscriber: DenisDvlp. martong added a comment. - Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/

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

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

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

NoQ accepted this revision. NoQ added a comment. Looks great as long as other reviewers are happy, thanks! Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409 - Optional FoundSummary = findFunctionSummary(FD, CE, C); + for (const

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

Szelethus added a comment. Just littering some more inlines, don't mind me :) Lets still wait on the dependency patch before updating. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296 +def StdCLibraryFunctionArgsChecker :

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

martong marked 2 inline comments as done. martong added a comment. In D73898#1901142 , @balazske wrote: > Is it sure that the signedness in the ranges is handled correctly? The EOF is > a negative value but the `RangeInt` is unsigned type. The >

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

martong marked 9 inline comments as done. martong added a comment. In D73898#1894923 , @balazske wrote: > It may be useful to make a "macro value map" kind of object. Some macros can > be added to it as a string, and it is possible to lookup for an

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

balazske added a comment. Is it sure that the signedness in the ranges is handled correctly? The EOF is a negative value but the `RangeInt` is unsigned type. The `tryExpandAsInteger` returns `int` too that is put into an unsigned `RangeInt` later. Probably it is better to use `APSInt` for the

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

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

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

balazske added a comment. something like this: class MacroUsageDetector { public: void addMacroName(StringRef MName); bool isMacroUsed(StringRef MName, Expr *E, ???); APSInt getMacroValue(StringRef MName); }; Or one that handles a single macro? Repository: rG LLVM Github

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

balazske added a comment. It may be useful to make a "macro value map" kind of object. Some macros can be added to it as a string, and it is possible to lookup for an `Expr` if one of the added macros is used there. This can be done by checking the concrete (numeric) value of the `Expr` and

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

gamesh411 added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697-699 + // The behavior is undefined if the value of the argument is not + // representable as unsigned char or is not equal to EOF. See e.g. C99 + //

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

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:152 +break; + default: +llvm_unreachable("Unknown RangeConstraint kind!"); This `default`

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

martong updated this revision to Diff 246229. martong added a comment. Rebase on top of https://reviews.llvm.org/D74973 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/ https://reviews.llvm.org/D73898 Files:

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

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:418 +if (FailureSt && !SuccessSt) { + C.addTransition(FailureSt); + if (ExplodedNode *N = C.generateErrorNode(FailureSt)) balazske wrote: >

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

martong updated this revision to Diff 246193. martong marked 7 inline comments as done. martong added a comment. - Use StringRef for Msg - Remove superfluous addTransition Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/

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

balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:418 +if (FailureSt && !SuccessSt) { + C.addTransition(FailureSt); + if (ExplodedNode *N = C.generateErrorNode(FailureSt)) Is this

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

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:407 +// FIXME Add detailed diagnostic. +std::string Msg = "Function argument constraint is not satisfied"; +auto R = std::make_unique(BT, Msg, N);

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

martong updated this revision to Diff 245652. martong added a comment. - Remove leftover call from test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/ https://reviews.llvm.org/D73898 Files:

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

martong added a comment. I've done a major refactor with the handling of argument constraints. I am now reusing `ValueRange::apply` in `checkPreCall` on "negated" value ranges to check if the constraint is violated. Comment at:

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

martong updated this revision to Diff 245651. martong marked 8 inline comments as done. martong added a comment. - Add new Checker that does the report - Refactor with negated RangeValues - Add overload to findFunctionSummary - Add tests for symbolic values - Add test file for bug path

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

balazske added a comment. Probably a better solution can be: For every "case" build a single `SVal` that contains all argument constraints for that case. It is possible using multiple `evalBinOp` calls (with <=, >=, logical or) to build such a condition (or repeated calls to other assume

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

balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:403 +return; + case nonloc::ConcreteIntKind: { + This check works now with concrete int values. We have a known value and a list of ranges with

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

martong updated this revision to Diff 244430. martong added a comment. - Rebase to master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73898/new/ https://reviews.llvm.org/D73898 Files:

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

martong planned changes to this revision. martong added a subscriber: steakhal. martong added a comment. Based on our verbal discussion with @Szelethus and @steakhal and based on the mailing archives , I am going to do the

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

martong added a comment. > What I mean by that is that we must do over-approximation if the argument is > symbolic. I.e. we presume that the constraints do hold otherwise the program > would be ill-formed and there is no point to continue the analysis on this > path. Sorry, that's actually

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

martong added a comment. In D73898#1864066 , @martong wrote: > In D73898#1863710 , @Szelethus wrote: > > > I wouldn't like to see reports emitted by a checker that resides in > > `apiModeling`. Could we create a

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

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697-699 + // The behavior is undefined if the value of the argument is not + // representable as unsigned char or is

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

martong added a comment. In D73898#1863710 , @Szelethus wrote: > I wouldn't like to see reports emitted by a checker that resides in > `apiModeling`. Could we create a new one? Some checkers, like the > `IteratorChecker`, `MallocChecker` and

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

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I wouldn't like to see reports emitted by a checker that resides in `apiModeling`. Could we create a new one? Some checkers, like the `IteratorChecker`, `MallocChecker` and

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:401 +return; + case nonloc::ConcreteIntKind: { +const llvm::APSInt = V.castAs().getValue(); Dealing with only concrete ints might be a good

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:411 + if (ExplodedNode *N = C.generateErrorNode(State)) { +// FIXME Add detailed diagnostic. +std::string Msg = "Function argument constraint is not

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

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62418 tests passed, 0 failed and 845 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

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

martong created this revision. martong added a reviewer: NoQ. Herald added subscribers: cfe-commits, Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a project: clang. Repository: rG LLVM