[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

balazske added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857
+// int fgetpos(FILE *restrict stream, fpos_t *restrict pos);
+// From 'The Open Group Base Specifications Issue 7, 2018 edition':
+// "The fgetpos() function shall not change the setting of errno if
+// successful."
+addToFunctionSummaryMap(
+"fgetpos",
+Signature(ArgTypes{FilePtrRestrictTy, FPosTPtrRestrictTy},

Szelethus wrote:
> martong wrote:
> > It is very good to see these new summaries! Would they be meaningful and 
> > usable without the changes in the StreamChecker? If yes, then I think we 
> > should split this into 2 patches.
> Can you please do this?
This is done now. There were 2 consecutive patches that modify 
`StdLibraryFunctionsChecker` and other checkers. These are rearranged into 2 
new patches. One for all changes in `StdLibraryFunctionsChecker` (and related 
changes in ErrnoModeling): D140387. Other for all other changes, 
`StreamChecker` and many of the tests that exercise both checkers: D140395.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

Szelethus added a comment.

In D135360#3981420 , @balazske wrote:

> My current approach is that the POSIX is more strict than the C standard 
> (POSIX allows a subset of what C allows). I do not see (errno related) 
> contradiction between these standards

Alright, so if we wanna support the C standard thinking, we would just create 
an option that would be a little more noisy. Sounds fair enough.

> (except initialization to zero of errno missing from POSIX, and possible 
> negative value in errno in POSIX). One problem exists now, that `errno` is 
> set to a non-zero (but not positive only) value at error. Probably we should 
> change this to be always positive at error, according to the C standard 
> `errno` is positive only, and POSIX does not tell that errno can be negative.

According to this (which I assume you've seen as well): 
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

> Description
> ===
>
> [...]
> Some of the functionality described on this reference page extends the ISO C 
> standard. Any conflict between the requirements described here and the ISO C 
> standard is unintentional. [...]
> [...]
>
> Issue 6
> ===
>
> Values for errno are now required to be distinct positive values rather than 
> non-zero values. This change is for alignment with the ISO/IEC 9899:1999 
> standard.

So yes, I'd say its reasonable to assume in POSIX modthat if `errno` is 
non-zero, it is **positive**.

> The checker currently works only in POSIX mode for every function, the 
> **ModelPOSIX** setting controls only if additional functions are added (all 
> with POSIX rules) (these functions could be added with C rules too).

I see a lot of mention of POSIX vs. the C standard, yet there seems to be no 
mention of these differences in the comments added by this patch or already 
commited. Maybe we could do better on documenting this important difference in 
behaviour.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857
+// int fgetpos(FILE *restrict stream, fpos_t *restrict pos);
+// From 'The Open Group Base Specifications Issue 7, 2018 edition':
+// "The fgetpos() function shall not change the setting of errno if
+// successful."
+addToFunctionSummaryMap(
+"fgetpos",
+Signature(ArgTypes{FilePtrRestrictTy, FPosTPtrRestrictTy},

martong wrote:
> It is very good to see these new summaries! Would they be meaningful and 
> usable without the changes in the StreamChecker? If yes, then I think we 
> should split this into 2 patches.
Can you please do this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

balazske added a comment.

My current approach is that the POSIX is more strict than the C standard (POSIX 
allows a subset of what C allows). I do not see (errno related) contradiction 
between these standards (except initialization to zero of errno missing from 
POSIX, and possible negative value in errno in POSIX). The checker currently 
works only in POSIX mode for every function, the **ModelPOSIX** setting 
controls only if additional functions are added (all with POSIX rules) (these 
functions could be added with C rules too). One problem exists now, that 
`errno` is set to a non-zero (but not positive only) value at error. Probably 
we should change this to be always positive at error, according to the C 
standard `errno` is positive only, and POSIX does not tell that errno can be 
negative.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

Szelethus added a comment.

In D135360#3862260 , @martong wrote:

> In D135360#3839890 , @balazske 
> wrote:
>
>> I found some anomalies during development:
>>
>> - If the checker **StdCLibraryFunctions** is added as dependency for 
>> **alpha.unix.Stream** in //checkers.td// I get some "unexplainable" test 
>> failures.
>
> Could you please elaborate? I don't see how to help you with it without 
> seeing more details.

Mind that dependencies also establish the order of callbacks (dependent 
checkers are called after their dependencies).

> When it is ambiguous then I'd check the latest standards of both POSIX and C. 
> If it is still doubtful then I'd vote for the C standard and would report a 
> defect towards the POSIX community.



In D135360#3888632 , @balazske wrote:

> About the "ftell" problem: The POSIX rules are really an extension of the C 
> standard rules. At `ftell` according to C standard `errno` should be set to a 
> positive value if error occurs. The POSIX rules extend this: `errno` is not 
> changed if no error occurs. [...] It may be best to use the POSIX rules for 
> the checker, because the C standard does not say much and may need to require 
> setting of `errno` to 0 before a standard function call.

Interesting pair of perspectives, I think a reasonable checker should be a 
little more conservative, more akin to what POSIX seems to specify.

In D135360#3885556 , @martong wrote:

> In D135360#3885494 , @balazske 
> wrote:
>
>> [...] Probably this should be a discourse question?
>
> Okay then, I think it is worth to have a discourse question. But you could 
> ask the wider "Clang" community, so I would not post the question as 
> something that is related strictly to the static analyzer.

Did this thread ever materialize? I admit I didn't follow :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

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

Rebase to main and D137722 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

Files:
  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/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- clang/test/Analysis/stream-noopen.c
+++ clang/test/Analysis/stream-noopen.c
@@ -77,6 +77,49 @@
   clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
 }
 
+void check_fgetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_fsetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_ftell(FILE *F) {
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  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}}
Index: clang/test/Analysis/stream-errno.c
===
--- clang/test/Analysis/stream-errno.c
+++ clang/test/Analysis/stream-errno.c
@@ -123,6 +123,64 @@
   fclose(F);
 }
 
+void check_fgetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_fsetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_ftell(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_rewind(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  rewind(F);
+  clang_analyzer_eval(errno == 0);
+  // expected-warning@-1{{FALSE}}
+  // expected-warning@-2{{TRUE}}
+  fclose(F);
+}
+
 void check_fileno(void) {
   FILE *F = tmpfile();
   if (!F)
Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -98,6 +98,18 @@
   (void)fclose(F);
 }
 
+void check_rewind_errnocheck(void) {
+  FILE *F = tmpfile();
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+return;
+  errno = 0;
+  rewind(F); // expected-note{{Function 'rewind' indicates failure only by setting of 'errno'}}
+  fclose(F); // expected-warning{{Value of 'errno' was not checked and may be overwritten by function 'fclose' [alpha.unix.Errno]}}
+  // expected-note@-1{{Value of 'errno' was not checked and may be overwritten by function 'fclose'}}
+}
+
 void check_fileno(void) {
   FILE *F = tmpfile();
   // expected-note@+2{{'F' is non-null}}
Index: 

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

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

Added more functions to StdLibraryFunctionsChecker.
This is redundant with the StreamChecker for now but
makes possible to remove the NULL stream check from
StreamChecker and reduce overlap of the checkers.
This way no dependency should be needed between these checkers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

Files:
  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/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- clang/test/Analysis/stream-noopen.c
+++ clang/test/Analysis/stream-noopen.c
@@ -77,6 +77,49 @@
   clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
 }
 
+void check_fgetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_fsetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_ftell(FILE *F) {
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  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}}
Index: clang/test/Analysis/stream-errno.c
===
--- clang/test/Analysis/stream-errno.c
+++ clang/test/Analysis/stream-errno.c
@@ -123,6 +123,64 @@
   fclose(F);
 }
 
+void check_fgetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_fsetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_ftell(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_rewind(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  rewind(F);
+  clang_analyzer_eval(errno == 0);
+  // expected-warning@-1{{FALSE}}
+  // expected-warning@-2{{TRUE}}
+  fclose(F);
+}
+
 void check_fileno(void) {
   FILE *F = tmpfile();
   if (!F)
Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -98,6 +98,18 @@
   (void)fclose(F);
 }
 
+void check_rewind_errnocheck(void) {
+  FILE *F = tmpfile();
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+return;
+  errno = 0;
+  rewind(F); // expected-note{{Function 'rewind' indicates failure only by setting of 'errno'}}
+  fclose(F); // expected-warning{{Value of 'errno' was not checked and may be overwritten by function 'fclose' [alpha.unix.Errno]}}
+  // 

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

balazske added a comment.

About the "ftell" problem: The POSIX rules are really an extension of the C 
standard rules. At `ftell` according to C standard `errno` should be set to a 
positive value if error occurs. The POSIX rules extend this: `errno` is not 
changed if no error occurs. This can be correct, then the CERT `ftell` 
non-compliant example is wrong (on success `errno` remains always 0). It can be 
fixed if line `errno=0` is removed from the start of the non-compliant code. It 
may be best to use the POSIX rules for the checker, because the C standard does 
not say much and may need to require setting of `errno` to 0 before a standard 
function call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D135360#3885494 , @balazske wrote:

> If we look only at the C standard we can not tell much about if the functions 
> should set `errno`. It seems that setting `errno` is totally 
> implementation-dependent, except for a few functions that mention `errno`. 
> These are `fgetpos`, `fsetpos`, `ftell` and should set `errno` to a positive 
> value on error (but there is no guarantee that value of `errno` is preserved 
> if no error occurs). And `errno` is always positive. This is different than 
> what the checker currently does (with the new patches). Probably this should 
> be a discourse question?

Okay then, I think it is worth to have a discourse question. But you could ask 
the wider "Clang" community, so I would not post the question as something that 
is related strictly to the static analyzer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

balazske added a comment.

If we look only at the C standard we can not tell much about if the functions 
should set `errno`. It seems that setting `errno` is totally 
implementation-dependent, except for a few functions that mention `errno`. 
These are `fgetpos`, `fsetpos`, `ftell` and should set `errno` to a positive 
value on error (but there is no guarantee that value of `errno` is preserved if 
no error occurs). And `errno` is always positive. This is different than what 
the checker currently does (with the new patches). Probably this should be a 
discourse question?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857
+// int fgetpos(FILE *restrict stream, fpos_t *restrict pos);
+// From 'The Open Group Base Specifications Issue 7, 2018 edition':
+// "The fgetpos() function shall not change the setting of errno if
+// successful."
+addToFunctionSummaryMap(
+"fgetpos",
+Signature(ArgTypes{FilePtrRestrictTy, FPosTPtrRestrictTy},

It is very good to see these new summaries! Would they be meaningful and usable 
without the changes in the StreamChecker? If yes, then I think we should split 
this into 2 patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D135360#3839890 , @balazske wrote:

> I found some anomalies during development:
>
> - If the checker **StdCLibraryFunctions** is added as dependency for 
> **alpha.unix.Stream** in //checkers.td// I get some "unexplainable" test 
> failures.

Could you please elaborate? I don't see how to help you with it without seeing 
more details.

> - In the comments section at CERT ERR30-C 
> 
>  is pointed out that function `ftell` can change the value of `errno` if it 
> is successful. This is the "normal" expected behavior of all standard 
> functions unless the documentation tells other. But this place (search for 
> ftell)  tells that `ftell` 
> should not change `errno` if successful. The persons commented at the CERT 
> rule probably checked only one or more of the C standards. But there should 
> be no conflict between POSIX and C ("Any conflict between the requirements 
> described here and the ISO C standard is unintentional."). It would be 
> unconventional if different check rules are needed (and add of a global 
> analyzer option for POSIX or C mode).

When it is ambiguous then I'd check the latest standards of both POSIX and C. 
If it is still doubtful then I'd vote for the C standard and would report a 
defect towards the POSIX community.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

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

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

I found some anomalies during development:

- If the checker **StdCLibraryFunctions** is added as dependency for 
**alpha.unix.Stream** in //checkers.td// I get some "unexplainable" test 
failures.
- In the comments section at CERT ERR30-C 

 is pointed out that function `ftell` can change the value of `errno` if it is 
successful. This is the "normal" expected behavior of all standard functions 
unless the documentation tells other. But this place (search for ftell) 
 tells that `ftell` should 
not change `errno` if successful. The persons commented at the CERT rule 
probably checked only one or more of the C standards. But there should be no 
conflict between POSIX and C ("Any conflict between the requirements described 
here and the ISO C standard is unintentional."). It would be unconventional if 
different check rules are needed (and add of a global analyzer option for POSIX 
or C mode).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and 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.

Add support for functions 'fgetpos', 'fsetpos', 'ftell', 'rewind'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135360

Files:
  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/stream-errno-note.c
  clang/test/Analysis/stream-errno.c
  clang/test/Analysis/stream-noopen.c

Index: clang/test/Analysis/stream-noopen.c
===
--- clang/test/Analysis/stream-noopen.c
+++ clang/test/Analysis/stream-noopen.c
@@ -77,6 +77,49 @@
   clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
 }
 
+void check_fgetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_fsetpos(FILE *F) {
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+  if (errno) {} // no-warning
+  clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}}
+}
+
+void check_ftell(FILE *F) {
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+ // expected-warning@-1{{FALSE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  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}}
Index: clang/test/Analysis/stream-errno.c
===
--- clang/test/Analysis/stream-errno.c
+++ clang/test/Analysis/stream-errno.c
@@ -123,6 +123,64 @@
   fclose(F);
 }
 
+void check_fgetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fgetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_fsetpos(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  fpos_t Pos;
+  int Ret = fsetpos(F, );
+  if (Ret)
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_ftell(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  long Ret = ftell(F);
+  if (Ret == -1) {
+clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(Ret >= 0); // expected-warning{{TRUE}}
+  }
+  if (errno) {} // no-warning
+  fclose(F);
+}
+
+void check_rewind(void) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  errno = 0;
+  rewind(F);
+  clang_analyzer_eval(errno == 0);
+  // expected-warning@-1{{FALSE}}
+  // expected-warning@-2{{TRUE}}
+  fclose(F);
+}
+
 void check_fileno(void) {
   FILE *F = tmpfile();
   if (!F)
Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -98,6 +98,18 @@
   (void)fclose(F);
 }
 
+void check_rewind_errnocheck(void) {
+  FILE *F = tmpfile();
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+return;
+  errno = 0;
+  rewind(F); // expected-note{{Function 'rewind' indicates failure only by setting of 'errno'}}
+  fclose(F); // expected-warning{{Value of 'errno' was not checked