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

[Gabor Marton via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

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) {
+ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);

NoQ wrote:
> NoQ wrote:
> > martong wrote:
> > > NoQ wrote:
> > > > martong wrote:
> > > > > 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.
> > > > Wait, i misunderstood the code. It's even worse than that: you're 
> > > > adding transitions in a loop, so it'll cause state splits for every 
> > > > constraint. Because you do not intend to create multiple branches here, 
> > > > there needs to be exactly one `addTransition` performed every time 
> > > > `checkPreCall` is called. I.e., for now this code is breaking 
> > > > everything whenever there's more than one constraint, regardless of 
> > > > whether it's on the same argument.
> > > Yeah, that's a very good catch, thanks! I am going to prepare a patch to 
> > > fix this soon. My idea is to store the `SuccessSt` and apply the next 
> > > argument constraint on that. And once the loop is finished I'll have call 
> > > the `addTransition()`.
> > Yup, that's the common thing to do in such cases.
> While we're at it, could you try to come up with a runtime assertion that'll 
> help us prevent these mistakes?
> 
> Like, dunno, make `CheckerContext` crash whenever there's more than one 
> branch being added, and then add a method to opt out when it's actually 
> necessary to add more transitions (i.e., the user would say 
> `C.setMaxTransitions(2)` at the beginning of their checker callback whenever 
> they need to make a state split, defaulting to 1). It's a bit tricky because 
> i still want to allow multiple transitions when they allow one branch (i.e., 
> transitions chained together) but i think it'll take a lot of review anxiety 
> from me because it's a very dangerous mistake to make and for now code review 
> is the only way to catch it. So, yay, faster code reviews.
Hmm I see your point and I agree this would be a valuable sanity check. But if 
you don't mind I'd like to address this in a different and stand-alone patch 
(independently from the quick-fix https://reviews.llvm.org/D76790) because it 
does not seem to be trivial for me. 

My first concern is this: if we have `1` as the default value for 
`maxTranisitions` then we should add an extra `C.setMaxTransitions(N)` in every 
checker callback that does a state split, is that right?


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

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

[Artem Dergachev via Phabricator via cfe-commits]

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 = VC->apply(State, Call, Summary);

NoQ wrote:
> martong wrote:
> > NoQ wrote:
> > > martong wrote:
> > > > 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.
> > > Wait, i misunderstood the code. It's even worse than that: you're adding 
> > > transitions in a loop, so it'll cause state splits for every constraint. 
> > > Because you do not intend to create multiple branches here, there needs 
> > > to be exactly one `addTransition` performed every time `checkPreCall` is 
> > > called. I.e., for now this code is breaking everything whenever there's 
> > > more than one constraint, regardless of whether it's on the same argument.
> > Yeah, that's a very good catch, thanks! I am going to prepare a patch to 
> > fix this soon. My idea is to store the `SuccessSt` and apply the next 
> > argument constraint on that. And once the loop is finished I'll have call 
> > the `addTransition()`.
> Yup, that's the common thing to do in such cases.
While we're at it, could you try to come up with a runtime assertion that'll 
help us prevent these mistakes?

Like, dunno, make `CheckerContext` crash whenever there's more than one branch 
being added, and then add a method to opt out when it's actually necessary to 
add more transitions (i.e., the user would say `C.setMaxTransitions(2)` at the 
beginning of their checker callback whenever they need to make a state split, 
defaulting to 1). It's a bit tricky because i still want to allow multiple 
transitions when they allow one branch (i.e., transitions chained together) but 
i think it'll take a lot of review anxiety from me because it's a very 
dangerous mistake to make and for now code review is the only way to catch it. 
So, yay, faster code reviews.


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

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

[Artem Dergachev via Phabricator via cfe-commits]

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 = VC->apply(State, Call, Summary);

martong wrote:
> NoQ wrote:
> > martong wrote:
> > > 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.
> > Wait, i misunderstood the code. It's even worse than that: you're adding 
> > transitions in a loop, so it'll cause state splits for every constraint. 
> > Because you do not intend to create multiple branches here, there needs to 
> > be exactly one `addTransition` performed every time `checkPreCall` is 
> > called. I.e., for now this code is breaking everything whenever there's 
> > more than one constraint, regardless of whether it's on the same argument.
> Yeah, that's a very good catch, thanks! I am going to prepare a patch to fix 
> this soon. My idea is to store the `SuccessSt` and apply the next argument 
> constraint on that. And once the loop is finished I'll have call the 
> `addTransition()`.
Yup, that's the common thing to do in such cases.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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) {
+ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);

NoQ wrote:
> martong wrote:
> > 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.
> Wait, i misunderstood the code. It's even worse than that: you're adding 
> transitions in a loop, so it'll cause state splits for every constraint. 
> Because you do not intend to create multiple branches here, there needs to be 
> exactly one `addTransition` performed every time `checkPreCall` is called. 
> I.e., for now this code is breaking everything whenever there's more than one 
> constraint, regardless of whether it's on the same argument.
Yeah, that's a very good catch, thanks! I am going to prepare a patch to fix 
this soon. My idea is to store the `SuccessSt` and apply the next argument 
constraint on that. And once the loop is finished I'll have call the 
`addTransition()`.


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

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

[Artem Dergachev via Phabricator via cfe-commits]

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 = VC->apply(State, Call, Summary);

martong wrote:
> 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.
Wait, i misunderstood the code. It's even worse than that: you're adding 
transitions in a loop, so it'll cause state splits for every constraint. 
Because you do not intend to create multiple branches here, there needs to be 
exactly one `addTransition` performed every time `checkPreCall` is called. 
I.e., for now this code is breaking everything whenever there's more than one 
constraint, regardless of whether it's on the same argument.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D73898/new/

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,61 @@
+// Check the basic reporting/warning and the application of constraints.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify=report
+
+// Check the bugpath related to the reports.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify=bugpath
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = isalnum(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not 

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

[Gabor Marton via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,61 @@
+// Check the basic reporting/warning and the application of constraints.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify=report
+
+// Check the bugpath related to the reports.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify=bugpath
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = isalnum(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not 

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

[Gabor Marton via Phabricator via cfe-commits]

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:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify

Szelethus wrote:
> martong wrote:
> > Szelethus wrote:
> > > Hmm, why do we have 2 different test files that essentially do the same? 
> > > Shouldn't we only have a single one with `analyzer-output=text`?
> > No, I wanted to have two different test files to test two different things: 
> > (1) We do have the constraints applied (here we don't care about the 
> > warnings and the path)
> > (2) Check that we have a warning with the proper tracking and notes.
> What if we had different `-verify`s? 
> `clang/test/Analysis/track-conditions.cpp` is a great example.
Yeah, that's a very good approach, I just changed it like that. :)


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

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

[Kristóf Umann via Phabricator via cfe-commits]

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 test files, but I'll leave it up to you to 
address or ignore it.




Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify

martong wrote:
> Szelethus wrote:
> > Hmm, why do we have 2 different test files that essentially do the same? 
> > Shouldn't we only have a single one with `analyzer-output=text`?
> No, I wanted to have two different test files to test two different things: 
> (1) We do have the constraints applied (here we don't care about the warnings 
> and the path)
> (2) Check that we have a warning with the proper tracking and notes.
What if we had different `-verify`s? `clang/test/Analysis/track-conditions.cpp` 
is a great example.


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

[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 ) 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 , ExplodedNode *N, CheckerContext ) 
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

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

[Gabor Marton via Phabricator via cfe-commits]

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 SINCE LAST ACTION
  https://reviews.llvm.org/D73898/new/

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) // This path is no longer feasible.
+ret = x / y; // No warning here
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \

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

[Balázs Kéri via Phabricator via cfe-commits]

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 be true on every branch.

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.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:112
   const Summary ) const = 0;
+virtual ValueConstraintPtr negate() const {
+  llvm_unreachable("Not implemented");

Is it better done with `= 0`?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:297
+
+  void ReportBug(const CallEvent , ExplodedNode *N, CheckerContext ) 
const {
+if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])

This should be called `reportBug`.


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

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

[Kristóf Umann via Phabricator via cfe-commits]

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: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:298
 
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions, "

Just noticed, this checker still lies in the `apiModeling` package. Could we 
find a more appropriate place?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr;
   class ValueConstraint {

martong wrote:
> 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.
Sure, I'm sold.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:260
 
+  BugType BT{this, "Unsatisfied argument constraints", categories::LogicError};
+

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?


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

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

[Gabor Marton via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) // This path is no longer feasible.
+ret = x / y; // No warning here
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \

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

[Gabor Marton via Phabricator via cfe-commits]

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 (actually gives a compiler warning 
> too).
> This default branch is not needed here (actually gives a compiler warning 
> too).

I am not sure why I thought that that's not needed, actually we need that. 
(Perhaps an intermediate version returned in each cases.)


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

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

[Gabor Marton via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) // This path is no longer feasible.
+ret = x / y; // No warning here
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   

[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;
   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(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 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

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

[Artem Dergachev via Phabricator via cfe-commits]

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 ValueConstraintPtr& VC : Summary.ArgConstraints) {
+ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);

Maybe we should add an assertion that the same argument isn't specified 
multiple times.


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

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

[Kristóf Umann via Phabricator via cfe-commits]

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 : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,

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.
```



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+ValueRange negate() const {
+  ValueRange tmp(*this);

martong wrote:
> Szelethus wrote:
> > Maybe `complement` would be a better name? That sounds a lot more like a 
> > set operation. Also, this function highlights well that inheritance might 
> > not be the best solution here.
> Well, we check the argument constraint validity by trying to apply it's 
> logical negation. In case of a range inclusion this is being out of that 
> range. In case of non-null this is being null. And so on. The logic how we 
> try to check an argument constraint is the same in all cases of the different 
> constraints. And that is the point: in order to support a new kind of 
> constraint we just have to figure out how to "apply" and "negate" one 
> constraint. In my opinion this is a perfect case for polimorphism.
We agreed on inheritance in the previous patch, and regarding the name, sure, 
leave it as-is. :)



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr;
   class ValueConstraint {

How about `ValueConstraintRef`?


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 
> `tryExpandAsInteger` returns `int` too that is put into an unsigned 
> `RangeInt` later. Probably it is better to use `APSInt` for the ranges? (The 
> problem exists already before this change.)


That is not a problem, because finally in `apply` we use an `APSInt` that is 
constructed by considering the correct `T` type, e.g.:

  const llvm::APSInt  = BVF.getValue(R[I].first, T);

We could consider `RangeInt` as a buffer that is big enough to hold the 
representation of the range values. The concrete interpretation of the bits (as 
`T`) is done by `APSInt`.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 `Expr` if one 
> of the added macros is used there. This can be done by checking the concrete 
> (numeric) value of the `Expr` and compare to the value of the macro, or by 
> checking if the expression comes from a macro and take this macro name (use 
> string comparison). Such an object can be useful because the functionality is 
> needed at more checkers, for example the ones I am working on (StreamChecker 
> and ErrorReturnChecker too).


Please see my previous answer to @gamesh411




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:
> How about we add an example as well?
You mean like NonNull or other constraints?



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
+  // 7.4.1.2 The isalpha function (p: 181-182).

gamesh411 wrote:
> martong wrote:
> > Szelethus wrote:
> > > This is true for the rest of the summaries as well, but shouldn't we 
> > > retrieve the `unsigned char` size from `ASTContext`?
> > Yes this is a good idea. I will do this.
> > 
> > What bothers me really much, however, is that we should handle EOF in a 
> > platform dependent way as well ... and I have absolutely no idea how to do 
> > that given that is defined by a macro in a platform specific header file. I 
> > am desperately in need for help and ideas about how could we get the value 
> > of EOF for the analysed platform.
> If the EOF is not used in the TU analyzed, then there would be no way to find 
> the specific `#define`.
> Another approach would be to check if the value is defined by an expression 
> that is the EOF define (maybe transitively?).
I believe that the given standard C lib implementation (e.g. glibc) must 
provide a header for the prototypes of these functions where EOF is also 
defined transitively in any of the dependent system headers. Otherwise user 
code could misuse the value of EOF and thus the program would behave in an 
undefined manner.

C99 clearly states that you should #include  to use isalhpa.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+ValueRange negate() const {
+  ValueRange tmp(*this);

Szelethus wrote:
> Maybe `complement` would be a better name? That sounds a lot more like a set 
> operation. Also, this function highlights well that inheritance might not be 
> the best solution here.
Well, we check the argument constraint validity by trying to apply it's logical 
negation. In case of a range inclusion this is being out of that range. In case 
of non-null this is being null. And so on. The logic how we try to check an 
argument constraint is the same in all cases of the different constraints. And 
that is the point: in order to support a new kind of constraint we just have to 
figure out how to "apply" and "negate" one constraint. In my opinion this is a 
perfect case for polimorphism.



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.

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.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:10
 // This checker improves modeling of a few simple library functions.
 // It does not generate warnings.
 //

Szelethus wrote:
> I suspect this comment is no longer relevant.
Uh, yes.



Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify

Szelethus wrote:
> Hmm, why do we have 2 different test files that essentially do the same? 
> Shouldn't we only have a single 

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

[Balázs Kéri via Phabricator via cfe-commits]

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 ranges?


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

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

[Kristóf Umann via Phabricator via cfe-commits]

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 after the dependency patch is agreed upon.




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]>,

How about we add an example as well?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+ValueRange negate() const {
+  ValueRange tmp(*this);

Maybe `complement` would be a better name? That sounds a lot more like a set 
operation. Also, this function highlights well that inheritance might not be 
the best solution here.



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.

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?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:10
 // This checker improves modeling of a few simple library functions.
 // It does not generate warnings.
 //

I suspect this comment is no longer relevant.



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(BT, Msg, N);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);

While I find your usage of lambdas fascinating, this one seems a bit 
unnecessary :)



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:402
+  return;
+// FIXME Add detailed diagnostic.
+StringRef Msg = "Function argument constraint is not satisfied";

That is a `TODO`, rather :^)



Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify

Hmm, why do we have 2 different test files that essentially do the same? 
Shouldn't we only have a single one with `analyzer-output=text`?



Comment at: clang/test/Analysis/std-c-library-functions.c:1-31
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify

What a beautiful sight. Thanks.


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

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

[Balázs Kéri via Phabricator via cfe-commits]

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 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

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

[Balázs Kéri via Phabricator via cfe-commits]

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 compare to the value of the macro, or by 
checking if the expression comes from a macro and take this macro name (use 
string comparison). Such an object can be useful because the functionality is 
needed at more checkers, for example the ones I am working on (StreamChecker 
and ErrorReturnChecker too).


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

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

[Endre Fülöp via Phabricator via cfe-commits]

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
+  // 7.4.1.2 The isalpha function (p: 181-182).

martong wrote:
> Szelethus wrote:
> > This is true for the rest of the summaries as well, but shouldn't we 
> > retrieve the `unsigned char` size from `ASTContext`?
> Yes this is a good idea. I will do this.
> 
> What bothers me really much, however, is that we should handle EOF in a 
> platform dependent way as well ... and I have absolutely no idea how to do 
> that given that is defined by a macro in a platform specific header file. I 
> am desperately in need for help and ideas about how could we get the value of 
> EOF for the analysed platform.
If the EOF is not used in the TU analyzed, then there would be no way to find 
the specific `#define`.
Another approach would be to check if the value is defined by an expression 
that is the EOF define (maybe transitively?).


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

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

[Gabor Marton via Phabricator via cfe-commits]

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` branch is not needed here (actually gives a compiler warning 
too).


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

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

[Gabor Marton via Phabricator via cfe-commits]

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:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) // This path is no longer feasible.
+ret = x / y; // No warning here
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify
+
+void 

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

[Gabor Marton via Phabricator via cfe-commits]

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:
> Is this `addTransition` needed? It would be OK to call `generateErrorNode` 
> with `State`. Even if not, adding the transition before should not be needed?
Yes, you are right it is superfluous, I removed it.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:688
   // locale-specific return values.
   .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
   .Case({ArgumentCondition(0U, OutOfRange,

balazske wrote:
> Why is this `{128, UCharMax}` here and at the next entry needed?
This is the local specific range , [128, 255]. There are characters like `ä` 
which we don't know if they are treated as an alphanumerical character or not. 
We can't really tell how a specific libc implementation classifies them. On the 
other hand, with English letters we can state the classes confidently.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:696
+  .ArgConstraint(ArgumentCondition(
+  0U, WithinRange, {{EOFv, EOFv}, {0, UCharMax}}))},
   },

balazske wrote:
> Is this `ArgConstraint` intentionally added only to `isalnum`?
> 
Yes, I wanted to create first the infrastructure and then later to add all 
these constraints to the rest of the summaries with new tests.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D73898

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c


Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -37,10 +37,8 @@
   y = 0;
   clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
 
-  if (x > 255) { // This path is no longer 
feasible.
-ret = isalnum(x);
+  if (x > 255) // This path is no longer feasible.
 ret = x / y; // No warning here
-  }
 
   ret = x / y; // expected-warning{{Division by zero}}
 }
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -404,7 +404,7 @@
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
 // FIXME Add detailed diagnostic.
-std::string Msg = "Function argument constraint is not satisfied";
+StringRef Msg = "Function argument constraint is not satisfied";
 auto R = std::make_unique(BT, Msg, N);
 bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
 C.emitReport(std::move(R));
@@ -415,8 +415,7 @@
 ProgramStateRef FailureSt = VR.negate().apply(State, Call, Summary);
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
-  C.addTransition(FailureSt);
-  if (ExplodedNode *N = C.generateErrorNode(FailureSt))
+  if (ExplodedNode *N = C.generateErrorNode(State))
 Report(N);
   break;
 } else {


Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -37,10 +37,8 @@
   y = 0;
   clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
 
-  if (x > 255) { // This path is no longer feasible.
-ret = isalnum(x);
+  if (x > 255) // This path is no longer feasible.
 ret = x / y; // No warning here
-  }
 
   ret = x / y; // expected-warning{{Division by zero}}
 }
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -404,7 +404,7 @@
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
 // FIXME Add detailed diagnostic.
-std::string Msg = "Function argument constraint is not satisfied";
+StringRef Msg = "Function argument constraint is not satisfied";
 auto R = std::make_unique(BT, Msg, N);
 bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
 C.emitReport(std::move(R));
@@ -415,8 +415,7 @@
 ProgramStateRef FailureSt = VR.negate().apply(State, Call, Summary);
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
-  C.addTransition(FailureSt);
-  if (ExplodedNode *N = C.generateErrorNode(FailureSt))
+  if (ExplodedNode *N = C.generateErrorNode(State))
 Report(N);
   break;
 } else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

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 `addTransition` needed? It would be OK to call `generateErrorNode` with 
`State`. Even if not, adding the transition before should not be needed?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:688
   // locale-specific return values.
   .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
   .Case({ArgumentCondition(0U, OutOfRange,

Why is this `{128, UCharMax}` here and at the next entry needed?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:696
+  .ArgConstraint(ArgumentCondition(
+  0U, WithinRange, {{EOFv, EOFv}, {0, UCharMax}}))},
   },

Is this `ArgConstraint` intentionally added only to `isalnum`?



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

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

[Balázs Benics via Phabricator via cfe-commits]

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);

StringRef


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

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

[Gabor Marton via Phabricator via cfe-commits]

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:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) // This path is no longer feasible.
+ret = x / y; // No warning here
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int 

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

[Gabor Marton via Phabricator via cfe-commits]

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: 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 satisfied";

NoQ wrote:
> Yeah, a must-have for this check to be enabled by default would be to be able 
> to provide a specific warning message for every function. I guess we could 
> include them in the summaries as an extra argument of `ArgConstraint`.
What about warning messages with placeholders? E.g. "Argument constraint of 
{nth} argument of {fun} is not satisfied. The value is {v}, however it should 
be in {range}." 
There will be a bunch of functions whose warning message template would be the 
same. On the other hand some others could have different warnings, and that 
justifies the need for specialized warnings.
Still, I think the warning message in the summary should be optional, because 
otherwise it would be really hard to automatically add summaries from other 
sources (like from cppcheck).

No matter how it turns out, this should be handled in a different patch.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:414
+auto R = std::make_unique(BT, Msg, N);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+C.emitReport(std::move(R));

NoQ wrote:
> Let's test our notes.
> 
> That'll be especially important when we get to non-concrete values, because 
> the visitor might need to be expanded (or we might need a completely new 
> visitor).
Ok, I added a separate test file where the tests focus on the bug path.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions.c

Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -1,8 +1,34 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,46 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) {
+int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}}
+(void)ret;
+  }
+}
+
+void test_alnum_infeasible_path(int x, int y) {
+  int ret = isalnum(x);
+  y = 0;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+
+  if (x > 255) { // This path is no longer feasible.
+ret = isalnum(x);
+ret = x / y; // No warning here
+  }
+
+  ret = x / y; // expected-warning{{Division by zero}}
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   

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

[Balázs Kéri via Phabricator via cfe-commits]

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 
functions to cut off outer ranges). If the condition can be satisfied (by 
assume) add the new state, the condition for return value can be added here 
too. Repeat this for every different case. If no applicable case is found none 
of the conditions can be assumed, this means argument constraint error.


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

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

[Balázs Kéri via Phabricator via cfe-commits]

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 known limits, so testing for //in any of the ranges// does work 
the same way as testing for //out of all ranges//. And testing if the value is 
inside one of the ranges is more simple code.

But I think the symbolic evaluation with "eval" and "assume" functions would be 
more generic here (and more simple code). Then the way of cutting-of the bad 
ranges is usable (probably still there is other solution).



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:464
+}
+
 void StdLibraryFunctionsChecker::checkPostCall(const CallEvent ,

If `evalCall` is used it could be more simple to test and apply the constraints 
for arguments and return value in a "single step".


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

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

[Gabor Marton via Phabricator via cfe-commits]

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:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c

Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+typedef struct FILE FILE;
+#define EOF -1
+
+int isalnum(int);
+void test_alnum_concrete() {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -51,6 +51,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -61,7 +62,8 @@
 using namespace clang::ento;
 
 namespace {
-class StdLibraryFunctionsChecker : public Checker {
+class StdLibraryFunctionsChecker
+: public Checker {
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
@@ -142,6 +144,10 @@
   const CallEvent ,
   const Summary ) const;
 
+void checkAsWithinRange(ProgramStateRef State, const CallEvent ,
+const Summary , const BugType ,
+CheckerContext ) const;
+
   public:
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const Summary ) const {
@@ -155,6 +161,21 @@
   }
   llvm_unreachable("Unknown ValueRange kind!");
 }
+
+void check(ProgramStateRef State, const CallEvent ,
+   const Summary , const BugType ,
+   CheckerContext ) const {
+  switch (Kind) {
+  case OutOfRange:
+llvm_unreachable("Not implemented yet!");
+  case WithinRange:
+checkAsWithinRange(State, Call, Summary, BT, C);
+return;
+  case ComparesToArgument:
+llvm_unreachable("Not implemented yet!");
+  }
+  llvm_unreachable("Unknown ValueRange kind!");
+}
   };
 
   /// The complete list of ranges that defines a single branch.
@@ -163,10 +184,15 @@
   using ArgTypes = std::vector;
   using Ranges = std::vector;
 
-  /// Includes information about function prototype (which is necessary to
-  /// ensure we're modeling the right function and casting values properly),
-  /// approach to invalidation, and a list of branches - essentially, a list
-  /// of list of ranges - essentially, a list of lists of lists of segments.
+  /// Includes information about
+  ///   * function prototype (which is necessary to
+  /// ensure we're modeling the right function and casting values properly),
+  ///   * approach to invalidation,
+  ///   * 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.
+  /// If these constraints are not satisfied that means a fatal error
+  /// usually resulting in undefined behaviour.
   struct Summary {
 const ArgTypes ArgTys;
 const QualType RetTy;
@@ -181,6 +207,10 @@
   Cases.push_back(VRS);
   return *this;
 }
+Summary (ValueRange VR) {
+  ArgConstraints.push_back(VR);
+  return *this;
+}
 
   private:
 static void assertTypeSuitableForSummary(QualType T) {
@@ -216,6 +246,8 @@
   // lazily, and it doesn't change after initialization.
   mutable llvm::StringMap FunctionSummaryMap;
 
+  BugType BT{this, "Unsatisfied argument constraints", categories::LogicError};
+
   // Auxiliary functions to support ArgNo within all structures
   // in a unified manner.
   static QualType getArgType(const Summary , ArgNo ArgN) {

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

[Gabor Marton via Phabricator via cfe-commits]

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 following changes:

- Add a new checker that is implemented in the `StdLibraryFunctionsChecker` 
class.
- This new checker if switched on is responsible for emitting the warning. Even 
if this is turned off, the sink node is generated if the argument violates the 
given condition.
- This means, the new checker has the sole responsibility of emitting the 
warning, but nothing more.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 under-approximation because we elide paths.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 new one? Some checkers, like the 
> > `IteratorChecker`, `MallocChecker` and `CStringChecker` implement a variety 
> > of user-facing checkers within the same class, that is also an option, if 
> > creating a new checker class is too much of a hassle.
>
>
> ... And actually it makes sense to apply the argument constraints only if we 
> know for sure that they are not violated. ...


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. It is very similar to what we do in case of the DivZero or the NullDeref 
Checkers: if there is no violation (no warning) and the variable is symbolic 
then we constrain the value by the condition. E.g. in DivZero::checkPreStmt we 
have:

  // If we get here, then the denom should not be zero. We abandon the implicit
  // zero denom case for now.
  C.addTransition(stateNotZero);

Strictly speaking, these transitions should be part of the modeling then in 
this sense (and they should be in PostStmt?). Still they are not separated into 
a different checker.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 not equal to EOF. See e.g. C99
+  // 7.4.1.2 The isalpha function (p: 181-182).

Szelethus wrote:
> This is true for the rest of the summaries as well, but shouldn't we retrieve 
> the `unsigned char` size from `ASTContext`?
Yes this is a good idea. I will do this.

What bothers me really much, however, is that we should handle EOF in a 
platform dependent way as well ... and I have absolutely no idea how to do that 
given that is defined by a macro in a platform specific header file. I am 
desperately in need for help and ideas about how could we get the value of EOF 
for the analysed platform.


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 `CStringChecker` implement a variety 
> of user-facing checkers within the same class, that is also an option, if 
> creating a new checker class is too much of a hassle.


Yes, we could split the warning emitting part to a new checker. My concern with 
that is in that case we would have the argument constraining part in 
checkPostCall still in this checker, because that is part of the modelling. And 
actually it makes sense to apply the argument constraints only if we know for 
sure that they are not violated. The violation then would  be checked in the 
new checker, this seems a bit awkward to me. Because checking the violation of 
the constraints and applying the constraints seems to be a cohesive action to 
me. I mean it would not even make sense to turn off the warning checker, 
because then we'd be applying the constraints blindly.


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

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

[Kristóf Umann via Phabricator via cfe-commits]

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 `CStringChecker` implement a variety of 
user-facing checkers within the same class, that is also an option, if creating 
a new checker class is too much of a hassle.




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
+  // 7.4.1.2 The isalpha function (p: 181-182).

This is true for the rest of the summaries as well, but shouldn't we retrieve 
the `unsigned char` size from `ASTContext`?


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

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

[Gábor Horváth via Phabricator via cfe-commits]

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 start but we might want to 
handle symbolic cases in the future like:

```
if (v > 255) 
  return isalpha(v);
```

I am ok with not addig this in the first version but adding TODOs and test 
cases upfront cannot hurt.

So basivally, I was wondering if we should query the solver for the result 
instead of matching the sval kind and just early return if we do not want to 
support a specific kind.


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

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

[Artem Dergachev via Phabricator via cfe-commits]

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 satisfied";

Yeah, a must-have for this check to be enabled by default would be to be able 
to provide a specific warning message for every function. I guess we could 
include them in the summaries as an extra argument of `ArgConstraint`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:414
+auto R = std::make_unique(BT, Msg, N);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+C.emitReport(std::move(R));

Let's test our notes.

That'll be especially important when we get to non-concrete values, because the 
visitor might need to be expanded (or we might need a completely new visitor).


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Gabor Marton via Phabricator via cfe-commits]

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 Github Monorepo

https://reviews.llvm.org/D73898

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c

Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+typedef struct FILE FILE;
+#define EOF -1
+
+int isalnum(int);
+void test_alnum_concrete() {
+  int ret = isalnum(256); // expected-warning{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+  clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -51,6 +51,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -60,7 +61,8 @@
 using namespace clang::ento;
 
 namespace {
-class StdLibraryFunctionsChecker : public Checker {
+class StdLibraryFunctionsChecker
+: public Checker {
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
@@ -142,6 +144,10 @@
 applyAsComparesToArgument(ProgramStateRef State, const CallEvent ,
   const FunctionSummaryTy ) const;
 
+void checkAsWithinRange(ProgramStateRef State, const CallEvent ,
+const FunctionSummaryTy , const BugType ,
+CheckerContext ) const;
+
   public:
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const FunctionSummaryTy ) const {
@@ -155,6 +161,21 @@
   }
   llvm_unreachable("Unknown ValueRange kind!");
 }
+
+void check(ProgramStateRef State, const CallEvent ,
+   const FunctionSummaryTy , const BugType ,
+   CheckerContext ) const {
+  switch (Kind) {
+  case OutOfRange:
+llvm_unreachable("Not implemented yet!");
+  case WithinRange:
+checkAsWithinRange(State, Call, Summary, BT, C);
+return;
+  case ComparesToArgument:
+llvm_unreachable("Not implemented yet!");
+  }
+  llvm_unreachable("Unknown ValueRange kind!");
+}
   };
 
   /// The complete list of ranges that defines a single branch.
@@ -164,15 +185,21 @@
   using RetTypeTy = QualType;
   using RangesTy = std::vector;
 
-  /// Includes information about function prototype (which is necessary to
-  /// ensure we're modeling the right function and casting values properly),
-  /// approach to invalidation, and a list of branches - essentially, a list
-  /// of list of ranges - essentially, a list of lists of lists of segments.
+  /// Includes information about
+  ///   * function prototype (which is necessary to
+  /// ensure we're modeling the right function and casting values properly),
+  ///   * approach to invalidation,
+  ///   * 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.
+  /// If these constraints are not satisfied that means a fatal error
+  /// usually resulting in undefined behaviour.
   struct FunctionSummaryTy {
 const ArgTypesTy ArgTypes;
 const RetTypeTy RetType;
 const InvalidationKindTy InvalidationKind;
 RangesTy Ranges;
+ValueRangeSet ArgConstraints;
 
 FunctionSummaryTy(ArgTypesTy ArgTypes, RetTypeTy RetType,
   InvalidationKindTy InvalidationKind)
@@ -183,6 +210,10 @@
   Ranges.push_back(VRS);
   return *this;
 }
+FunctionSummaryTy (ValueRange VR) {
+  ArgConstraints.push_back(VR);
+  return *this;
+}
 
   private:
 static void