[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 483446.
balazske added a comment.

Updated test in errno-noopen.c.
The functions feof and ferror reset errno, the previous test was incorrect in 
this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-POSIX.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-error.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,114 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize_errno(FILE *F, int A) {
+  switch (A) {
+  case 1:
+fwrite(WBuf, 1, 0, F);
+if (errno) {} // expected-warning{{undefined}}
+break;
+  case 2:
+fwrite(WBuf, 0, 1, F);
+if (errno) {} // expected-warning{{undefined}}
+break;
+  case 3:
+fread(RBuf, 1, 0, F);
+if (errno) {} // expected-warning{{undefined}}
+break;
+  case 4:
+fread(RBuf, 0, 1, F);
+if (errno) {} // expected-warning{{undefined}}
+break;
+  }
+}
Index: 

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 483213.
balazske added a comment.

Removed change of errno state from StreamChecker.
Added cases for zero buffer size in `fread` and `fwrite`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-POSIX.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-error.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+}
Index: clang/test/Analysis/stream-error.c
===
--- clang/test/Analysis/stream-error.c
+++ clang/test/Analysis/stream-error.c
@@ -7,6 +7,7 @@
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
 void clang_analyzer_warnIfReached(void);
 void StreamTesterChecker_make_feof_stream(FILE *);
 void 

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 483106.
balazske marked an inline comment as done.
balazske edited the summary of this revision.
balazske added a comment.

Rebase to current main, fix of comment issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-POSIX.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+}
Index: clang/test/Analysis/stream-errno.c
===
--- /dev/null
+++ clang/test/Analysis/stream-errno.c
@@ -0,0 +1,138 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions,debug.ExprInspection \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true -verify %s
+
+#include 

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske marked 3 inline comments as done.
balazske added a comment.

In D135247#3993351 , @NoQ wrote:

> Also, similarly to `getenv()`, in these cases domain-specific knowledge may 
> help suppress some unwanted reports. Eg., if a file has been opened 
> successfully, this doesn't technically mean that it'll be open successfully 
> again, but it makes it more likely, and the user does not necessarily care 
> about time-of-check-time-of-use races. So maybe it'd make sense to eventually 
> move some of that stuff to StreamChecker anyway. Maybe not, hard to tell, 
> need to see the results.

This knowledge of "call history" can be implemented in an other checker, for 
the stream functions in `StreamChecker`, for `getenv` in a probably new checker 
(where the variable name could be stored). This `StdLibraryFunctionChecker` 
does not create the branch if the conditions (constraints) of a branch (summary 
case) are not satisfied. If another checker added branches in `evalCall` (for a 
success and failure case or only one of them) these are "selected" here only, 
not added.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1060-1064
+} else if (NewState == State) {
+  if (const auto *D = dyn_cast_or_null(Call.getDecl()))
+if (const NoteTag *NT =
+Case.getErrnoConstraint().describe(C, D->getNameAsString()))
+  C.addTransition(NewState, NT);

martong wrote:
> balazske wrote:
> > martong wrote:
> > > Why do we need this change?
> > It is possible that only the errno related state is changed, no new 
> > constraints are added (if the constraint is already here from `evalCall` 
> > but the errno was not set there, for example at `fclose` or other stream 
> > functions maybe no new state is created here). In such case the note tag is 
> > still needed.
> Okay, please add that as a comment to this new hunk.
The add of NoteTags could be improved. Probably a NoteTag should be displayed 
here if the return value (the "function call itself") is interesting. A text 
message should be specified for every function and the errno-related part added 
to it programatically if needed (if errno is interesting).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added a comment.

> Is there a plan to enable ModelPOSIX by default, at least on POSIX-compliant 
> target triples?

Yes, this would be more convenient. But all other modeled functions (not only 
in the ModelPOSIX part of the code) must be checked if they are POSIX 
compliant. The functions where there is difference between POSIX and non-POSIX 
(C standard only) can be added to the summary list in both cases with different 
summary. (The C standard draft specifies much less about how `errno` can be 
used.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Also, similarly to `getenv()`, in these cases domain-specific knowledge may 
help suppress some unwanted reports. Eg., if a file has been opened 
successfully, this doesn't technically mean that it'll be open successfully 
again, but it makes it more likely, and the user does not necessarily care 
about time-of-check-time-of-use races. So maybe it'd make sense to eventually 
move some of that stuff to StreamChecker anyway. Maybe not, hard to tell, need 
to see the results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I like where this is going. Is there a plan to enable `ModelPOSIX` by default, 
at least on POSIX-compliant target triples?

I'm somewhat worried that some users may find null dereference warnings on 
`fopen()` more annoying than useful (though they're definitely more valuable 
than, say, a null dereference warning over `malloc()` which can technically 
fail but in practice it just doesn't). In any case, these warnings will need to 
be properly explained with notes, otherwise it's very likely that reports are 
going to be hard to understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

Szelethus added a comment.

The patch looks OK now, I'll get to inspecting the others.

In D135247#3977403 , @balazske wrote:

> The "strange" test failures that showed up earlier were probably caused by a 
> bug that is fixed in the D137722 . I just 
> read that this patch is rebased to D137722  
> too, fixed the dependency stack.

Very well!

> There was another problem with circular dependencies (because 
> **StdCLibraryFunctionArgsChecker** had a dependency to StreamChecker, this is 
> removed in the last patch). The checker option must be not a problem, the 
> checker (`StdLibraryFunctionsChecker`) can be disabled (but is normally not 
> because it is an **apiModeling** checker) or the **ModelPOSIX** option turned 
> off independently if `StreamChecker` is enabled or not.

Okay, so the checker behaves OK if `StdLibraryFunctionsChecker` is disabled. As 
long as it doesn't crash, this is fine, you shouldn't disable it in practice 
anyways!

> The goal (should work at the end of this patch stack) is that StreamChecker 
> can report all bugs that it can find, and there is no case when both checkers 
> report a bug (in different way). If **ModelPOSIX** is turned off and 
> StreamChecker is enabled, for `fseek` for example no bug is found if stream 
> is NULL, and value of `errno` is just invalidated in all cases (like it works 
> if StreamChecker is disabled too), but the stream state and file position is 
> still checked by StreamChecker for all functions.

This sounds reasonable. It means that no parts of `StdLibraryFunctionsChecker` 
(including its option) is a "hard" dependency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added a comment.

The "strange" test failures that showed up earlier were probably caused by a 
bug that is fixed in the D137722 . I just 
read that this patch is rebased to D137722  
too, fixed the dependency stack. There was another problem with circular 
dependencies (because **StdCLibraryFunctionArgsChecker** had a dependency to 
StreamChecker, this is removed in the last patch). The checker option must be 
not a problem, the checker (`StdLibraryFunctionsChecker`) can be disabled (but 
is normally not because it is an **apiModeling** checker) or the **ModelPOSIX** 
option turned off independently if `StreamChecker` is enabled or not. The goal 
(should work at the end of this patch stack) is that StreamChecker can report 
all bugs that it can find, and there is no case when both checkers report a bug 
(in different way). If **ModelPOSIX** is turned off and StreamChecker is 
enabled, for `fseek` for example no bug is found if stream is NULL, and value 
of `errno` is just invalidated in all cases (like it works if StreamChecker is 
disabled too), but the stream state and file position is still checked by 
StreamChecker for all functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

Szelethus added a comment.

Sorry abour my previous reply, I messed up the thread I was replying to. I 
better see what is going on.

Do you have a better handle on @martong's previous comment (D135247#3867603 
)? Do we know why this strange 
behaviour occurs with StreamChecker? I assumed that the stream related modeling 
is done in parallel to that. Is it about the return value?

In D135247#3865357 , @balazske wrote:

> Probably if the `StdLibraryFunctionsChecker` object can be get from 
> `StreamChecker` (a new function is needed) it is possible to check the option 
> and use `CheckerManager::reportInvalidCheckerOptionValue` (for the 
> `StdLibraryFunctionsChecker`). But not sure if it does what we want here.

You can only retrieve a checker object if you can access the class definition 
of it (as you are aware), so unless you move `StdLibraryFunctionsChecker` to a 
header file, which doesn't seem like the correct option now (kinda goes against 
the independent module architecture  (despite us talking about intertwining 
these "independent" modules)).

For me, the right course seems to be to detach this option from 
`StdLibraryFunctionsChecker`, and make it an analyzer level option. If this 
option so severely impacts both checkers, it feels more like an analyzer level 
option anyways. That will enable you to query the option from 
`shouldRegisterStreamChecker`. Whether you'd prefer to hard error or only warn 
about the failure to enable a checker is something I also struggle with, but 
you could start out with the hard error first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:786
   /// constraints (and different return value constraints).
-  const NoErrnoConstraint ErrnoIrrelevant{};
+  const NoErrnoConstraint ErrnoUnchanged{};
+  const ResetErrnoConstraint ErrnoIrrelevant{};

Szelethus wrote:
> Hmm, do we need to specify this? Can't this be the default?
This is only used in cases where `errno` should not change (previous value 
remains, no invalidation happens). This works if another checker has `evalCall` 
for the function and does not change `errno` (otherwise a default evaluation 
occurs that invalidates the system region of `errno`).
The `StreamChecker` is an example with some functions added in the next patch. 
This class is really not used in this patch, could be moved to the next.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

Szelethus added a comment.

Do I understand correctly that for the time being, the strategy is to assume 
apiModeling to be enabled by default and not enforce it explicitly?

I suppose I have the ol' reliable question: did you evaluate this on any 
project yet?

In D135247#3839543 , @martong wrote:

>> The two checkers work together and 'apiModeling.StdCLibraryFunctions'
>> and its 'ModelPOSIX=true' option should be now a dependency of
>> checker 'alpha.unix.Stream'.
>
> Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not 
> set but the  'alpha.unix.Stream' checker is enabled? @Szelethus, maybe you 
> have some insights with this?

The problem is, Dependencies and Weakdependencies both enforce a strict order 
of checker registration. What is discussed here is the following order:

1. StdCLibraryFunctionsChecker (modelling part)
2. StreamChecker
3. StdCLibraryFunctionArgsChecker (reporting part)

This falls apart because the modeling and reporting checkers are, in fact, the 
same checker in implementation. The "proper" solution would be to split up 
StdLibraryFunctions not in terms of a new checker entry in Checkers.td, but to 
split up the checker in implementation.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:786
   /// constraints (and different return value constraints).
-  const NoErrnoConstraint ErrnoIrrelevant{};
+  const NoErrnoConstraint ErrnoUnchanged{};
+  const ResetErrnoConstraint ErrnoIrrelevant{};

Hmm, do we need to specify this? Can't this be the default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 474556.
balazske added a comment.

Rebase to main and D137722 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-POSIX.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+}
Index: clang/test/Analysis/stream-errno.c
===
--- /dev/null
+++ clang/test/Analysis/stream-errno.c
@@ -0,0 +1,138 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions,debug.ExprInspection \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_func.h"
+

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D135247#3867445 , @balazske wrote:

> If `StdCLibraryFunctionsChecker` is added as dependency to `StreamChecker` 
> and no other thing is changed these tests fail, and I could not find out the 
> reason:
> Errors in test **stream.c**:
>
>   error: 'warning' diagnostics expected but not seen: 
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 7: 
> Stream pointer might be NULL
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 
> 13: Stream pointer might be NULL
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 
> 76: Stream pointer might be NULL
>
> Errors in test **stream-error.c**:
>
>   error: 'warning' diagnostics expected but not seen: 
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 97: might be 'indeterminate'
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 100: Stream might be already closed
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 113: Read function called when stream is in EOF state
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 114: Read function called when stream is in EOF state
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 123: FALSE
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 124: FALSE
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 128: FALSE
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 129: TRUE
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 133: TRUE
> File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
> Line 134: FALSE
>
> Probably the `StdLibraryFunctionsChecker` runs before the `StreamChecker`, 
> and runs always after it if no dependency is there? But this does not explain 
> all test errors and should cause no errors at all. Adding the dependency 
> itself is not enough, `ModelPOSIX` option should be true too. If the option 
> is set to true in the test file even more test errors appear, and it does not 
> work even when the (StdLibraryFunctionsChecker) checker is added (to the RUN 
> command). Without the dependency the tests do not fail if the order of 
> checkers in the enabled checkers list is changed.

Okay, this is bad. We must understand the reasons behind these failures. It is 
very strange that the `ModelPOSIX` option triggers these failures. I think we 
have to hunt down and examine each failing check, could you please continue the 
debugging of these?




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1060-1064
+} else if (NewState == State) {
+  if (const auto *D = dyn_cast_or_null(Call.getDecl()))
+if (const NoteTag *NT =
+Case.getErrnoConstraint().describe(C, D->getNameAsString()))
+  C.addTransition(NewState, NT);

balazske wrote:
> martong wrote:
> > Why do we need this change?
> It is possible that only the errno related state is changed, no new 
> constraints are added (if the constraint is already here from `evalCall` but 
> the errno was not set there, for example at `fclose` or other stream 
> functions maybe no new state is created here). In such case the note tag is 
> still needed.
Okay, please add that as a comment to this new hunk.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:604-613
+  // Return 0 on success, EOF on failure.
+  SValBuilder  = C.getSValBuilder();
+  ProgramStateRef StateSuccess = State->BindExpr(
+  CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy));
+  ProgramStateRef StateFailure =
+  State->BindExpr(CE, C.getLocationContext(),
+  SVB.makeIntVal(*EofVal, C.getASTContext().IntTy));

balazske wrote:
> martong wrote:
> > This is redundant with the summary in the `StdLibraryFunctionsChecker`. Why 
> > do we need this as well?
> It is probably needed to have a (any) bound value to the `fclose` function 
> call, otherwise setting constraints in the other checker do not work. It may 
> work to bind only a conjured value but it looks better if the correct return 
> value is used. This makes less inter-dependence between the two checkers 
> (`StdLibraryFunctionChecker` sets only the errno state as far as possible).
Ok, makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added a comment.

If `StdCLibraryFunctionsChecker` is added as dependency to `StreamChecker` and 
no other thing is changed these tests fail, and I could not find out the reason:
Errors in test **stream.c**:

  error: 'warning' diagnostics expected but not seen: 
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 7: 
Stream pointer might be NULL
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 13: 
Stream pointer might be NULL
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 76: 
Stream pointer might be NULL

Errors in test **stream-error.c**:

  error: 'warning' diagnostics expected but not seen: 
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 97: might be 'indeterminate'
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 100: Stream might be already closed
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 113: Read function called when stream is in EOF state
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 114: Read function called when stream is in EOF state
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 123: FALSE
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 124: FALSE
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 128: FALSE
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 129: TRUE
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 133: TRUE
File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c 
Line 134: FALSE

Probably the `StdLibraryFunctionsChecker` runs before the `StreamChecker`, and 
runs always after it if no dependency is there? But this does not explain all 
test errors and should cause no errors at all. Adding the dependency itself is 
not enough, `ModelPOSIX` option should be true too. If the option is set to 
true in the test file even more test errors appear, and it does not work even 
when the (StdLibraryFunctionsChecker) checker is added (to the RUN command). 
Without the dependency the tests do not fail if the order of checkers in the 
enabled checkers list is changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added a comment.

Probably if the `StdLibraryFunctionsChecker` object can be get from 
`StreamChecker` (a new function is needed) it is possible to check the option 
and use `CheckerManager::reportInvalidCheckerOptionValue` (for the 
`StdLibraryFunctionsChecker`). But not sure if it does what we want here.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1060-1064
+} else if (NewState == State) {
+  if (const auto *D = dyn_cast_or_null(Call.getDecl()))
+if (const NoteTag *NT =
+Case.getErrnoConstraint().describe(C, D->getNameAsString()))
+  C.addTransition(NewState, NT);

martong wrote:
> Why do we need this change?
It is possible that only the errno related state is changed, no new constraints 
are added (if the constraint is already here from `evalCall` but the errno was 
not set there, for example at `fclose` or other stream functions maybe no new 
state is created here). In such case the note tag is still needed.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:604-613
+  // Return 0 on success, EOF on failure.
+  SValBuilder  = C.getSValBuilder();
+  ProgramStateRef StateSuccess = State->BindExpr(
+  CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy));
+  ProgramStateRef StateFailure =
+  State->BindExpr(CE, C.getLocationContext(),
+  SVB.makeIntVal(*EofVal, C.getASTContext().IntTy));

martong wrote:
> This is redundant with the summary in the `StdLibraryFunctionsChecker`. Why 
> do we need this as well?
It is probably needed to have a (any) bound value to the `fclose` function 
call, otherwise setting constraints in the other checker do not work. It may 
work to bind only a conjured value but it looks better if the correct return 
value is used. This makes less inter-dependence between the two checkers 
(`StdLibraryFunctionChecker` sets only the errno state as far as possible).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks for patch and the many test cases.

You still haven't answered my below concern:

> Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not 
> set but the 'alpha.unix.Stream' checker is enabled?




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1045
   NT = Case.getErrnoConstraint().describe(C, D->getNameAsString());
+  // llvm::errs()getNameAsString()))
+  C.addTransition(NewState, NT);

Why do we need this change?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1724-1797
+const auto ReturnsZeroOrMinusOne =
+ConstraintSet{ReturnValueCondition(WithinRange, Range(-1, 0))};
+const auto ReturnsZero =
+ConstraintSet{ReturnValueCondition(WithinRange, SingleValue(0))};
+const auto ReturnsMinusOne =
+ConstraintSet{ReturnValueCondition(WithinRange, SingleValue(-1))};
+const auto ReturnsNonnegative =

Very good!



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:604-613
+  // Return 0 on success, EOF on failure.
+  SValBuilder  = C.getSValBuilder();
+  ProgramStateRef StateSuccess = State->BindExpr(
+  CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy));
+  ProgramStateRef StateFailure =
+  State->BindExpr(CE, C.getLocationContext(),
+  SVB.makeIntVal(*EofVal, C.getASTContext().IntTy));

This is redundant with the summary in the `StdLibraryFunctionsChecker`. Why do 
we need this as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> The two checkers work together and 'apiModeling.StdCLibraryFunctions'
> and its 'ModelPOSIX=true' option should be now a dependency of
> checker 'alpha.unix.Stream'.

Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not 
set but the  'alpha.unix.Stream' checker is enabled? @Szelethus, maybe you have 
some insights with this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 465423.
balazske added a comment.

Fixed failing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-POSIX.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+}
Index: clang/test/Analysis/stream-errno.c
===
--- /dev/null
+++ clang/test/Analysis/stream-errno.c
@@ -0,0 +1,138 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions,debug.ExprInspection \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_func.h"
+
+extern void clang_analyzer_eval(int);
+extern 

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske added a reviewer: martong.
balazske added a comment.
Herald added a subscriber: rnkovacs.

This is another approach to D132017 . This 
eliminates redundant code from `StreamChecker` that can be done with 
`StdLibraryFunctionsChecker` too. Something to fix is to somehow mention the 
dependency of **apiModeling.StdCLibraryFunctions** and //ModelPOSIX=true// 
option. If `StreamChecker` is used without these dependencies the errno part of 
the modeling can be incorrect (errno is not changed by the `evalCall` in 
`StreamChecker`, it is expected to be updated in the other checker in 
`postCall`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

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

balazske created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, 
dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a reviewer: NoQ.
Herald added a project: All.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Some of the stream handling functions are modeled in StreamChecker.
These are now added to StdLibraryFunctionsChecker where additional
checks are performed on these functions. This includes setting of
'errno' related state. In this way the errno state is set for these
functions (almost) completely in StdLibraryFunctionsChecker.
The two checkers work together and 'apiModeling.StdCLibraryFunctions'
and its 'ModelPOSIX=true' option should be now a dependency of
checker 'alpha.unix.Stream'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135247

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- /dev/null
+++ clang/test/Analysis/stream-noopen.c
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// enable only StdCLibraryFunctions checker
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Errno \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/errno_var.h"
+
+void clang_analyzer_eval(int);
+
+const char *WBuf = "123456789";
+char RBuf[10];
+
+void test_freopen(FILE *F) {
+  F = freopen("xxx", "w", F);
+  if (F) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+}
+
+void test_fread(FILE *F) {
+  size_t Ret = fread(RBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fwrite(FILE *F) {
+  size_t Ret = fwrite(WBuf, 1, 10, F);
+  if (Ret == 10) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fclose(FILE *F) {
+  int Ret = fclose(F);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void test_fseek(FILE *F) {
+  int Ret = fseek(F, SEEK_SET, 1);
+  if (Ret == 0) {
+if (errno) {} // expected-warning{{undefined}}
+  } else {
+clang_analyzer_eval(Ret == -1); // expected-warning {{TRUE}}
+clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}}
+  }
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(WBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fwrite(WBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 1, 0, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+  if (errno) {} // no-warning
+  fread(RBuf, 0, 1, F);
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+