[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39670ae3b934: [clang][analyzer] Add and change NoteTags in 
StdLibraryFunctionsChecker. (authored by balazske).

Changed prior to commit:
  https://reviews.llvm.org/D153612?vs=540015=541353#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{'fclose' fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{'freopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{'fopen' fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,13 +107,16 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 clearerr(F);
 fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+// stdargs-note@-1 {{'fread' fails}}
 if (feof(F)) { // expected-note {{Taking true branch}}
   fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
   // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
@@ -115,10 +129,12 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' is successful}}
   if (feof(F)) { // expected-note {{Taking false branch}}
 fclose(F);
 return;
@@ -127,6 +143,7 @@
 return;
   }
   fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

steakhal added a comment.

In D153612#4505307 , @balazske wrote:

> No major problems were indicated with the patch stack, I plan to merge all of 
> these soon. Small problems can be still corrected before or when the checker 
> is put out from the alpha package.

Make sure features are consistent as clang-17 will branch off next Tuesday if 
you want these to be part of that release.
Also, consider fine-tuning the proposed release-notes for the features you 
developed. See D155445 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske added a comment.

No major problems were indicated with the patch stack, I plan to merge all of 
these soon. Small problems can be still corrected before or when the checker is 
put out from the alpha package.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 540015.
balazske marked 6 inline comments as done.
balazske added a comment.

Changed format string back to `str().c_str()`,
changed `dyn_cast_or_null`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{'fclose' fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{'freopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{'fopen' fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,13 +107,16 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 clearerr(F);
 fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+// stdargs-note@-1 {{'fread' fails}}
 if (feof(F)) { // expected-note {{Taking true branch}}
   fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
   // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
@@ -115,10 +129,12 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' is successful}}
   if (feof(F)) { // expected-note {{Taking false branch}}
 fclose(F);
 return;
@@ -127,6 +143,7 @@
 return;
   }
   fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

(Just added some side remarks about string handling annoyances.)

@balazske Please (1) handle the `dyn_cast` request of  @steakhal and also (2) 
do something with this "how to convert `StringRef` to `char *`" question that 
we're bikeshedding. I hope that after those we could finally merge this commit 
sequence.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305
+  std::string Note =
+  llvm::formatv(Case.getNote().str().c_str(),
+cast(Call.getDecl())->getDeclName());

steakhal wrote:
> donat.nagy wrote:
> > Consider using the method `StringRef::data()` which directly converts a 
> > `StringRef` to a `const char *`. Your two-step conversion has the advantage 
> > that it adds a `\0` terminator even if the `StringRef` isn't 
> > null-terminated, but I cannot imagine a "natural" code change that would 
> > introduce references to non-null-terminated char arrays as note message 
> > templates.
> I would prefer not to rely on that `StringRef`s (here) are expected to be 
> null-terminated, unless benchmarks demonstrate that this is important. If 
> that would turn out to be the case, then we would need to enforce this using 
> some sort of `assert` expression.
I think the way of expressing this concrete conversion is a very low priority 
question (a.k.a. bikeshedding), so returning to the previously used two-step 
conversion is also fine for me.

The primary goal of this ".data()" shortcut was not performance, but clarity: 
those chained conversions triggered my "detect suspicious code" instincts and 
this was the best alternative that I could find. In general I feel that the 
LLVM codebase has way too many string types, and the back-and-forth conversions 
between them add lots of unnecessary noise to the code.

In this particular case I think it's the fault of `llvm::formatv` that it 
doesn't accept `StringRef` (the most commonly used non-owning string) and in 
general it's problematic design that there is no "good" conversion between 
`StringRef` and C-style strings in either direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

steakhal added inline comments.



Comment at: clang/test/Analysis/stream-note.c:61-62
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)

steakhal wrote:
> balazske wrote:
> > steakhal wrote:
> > > Why are these notes doubled?
> > There are 2 cases of resource leak reported (for `F1` and `F2`) and a note 
> > tag is there for both of these.
> I still think we should only have a single note there given that `F2` is 
> completely unrelated to the initialization of `F1`.
> Do I miss something?
Oo, now I see. D153776 is supposed to fix limit the notes to be displayed only 
for the interesting values. nvm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

steakhal added inline comments.



Comment at: clang/test/Analysis/stream-note.c:61-62
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)

balazske wrote:
> steakhal wrote:
> > Why are these notes doubled?
> There are 2 cases of resource leak reported (for `F1` and `F2`) and a note 
> tag is there for both of these.
I still think we should only have a single note there given that `F2` is 
completely unrelated to the initialization of `F1`.
Do I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

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



Comment at: clang/test/Analysis/stream-note.c:61-62
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)

steakhal wrote:
> Why are these notes doubled?
There are 2 cases of resource leak reported (for `F1` and `F2`) and a note tag 
is there for both of these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

steakhal added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305
+  std::string Note =
+  llvm::formatv(Case.getNote().str().c_str(),
+cast(Call.getDecl())->getDeclName());

donat.nagy wrote:
> Consider using the method `StringRef::data()` which directly converts a 
> `StringRef` to a `const char *`. Your two-step conversion has the advantage 
> that it adds a `\0` terminator even if the `StringRef` isn't null-terminated, 
> but I cannot imagine a "natural" code change that would introduce references 
> to non-null-terminated char arrays as note message templates.
I would prefer not to rely on that `StringRef`s (here) are expected to be 
null-terminated, unless benchmarks demonstrate that this is important. If that 
would turn out to be the case, then we would need to enforce this using some 
sort of `assert` expression.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1337
+// if 'errno' is interesting.
+if (const auto *D = dyn_cast_or_null(Call.getDecl()))
+  if (const NoteTag *NT =

Previously, we had a `cast<>(Call.getDecl())`, thus we should only have a 
`dyn_cast` here.



Comment at: clang/test/Analysis/stream-note.c:61-62
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)

Why are these notes doubled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

steakhal added a comment.

Please, let me have a look at this stack tomorrow or the day after prior 
landing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

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

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{'fclose' fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{'freopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{'fopen' fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,13 +107,16 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 clearerr(F);
 fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+// stdargs-note@-1 {{'fread' fails}}
 if (feof(F)) { // expected-note {{Taking true branch}}
   fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
   // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
@@ -115,10 +129,12 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' is successful}}
   if (feof(F)) { // expected-note {{Taking false branch}}
 fclose(F);
 return;
@@ -127,6 +143,7 @@
 return;
   }
   fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

See results and their discussion on the review of the tightly connected 
follow-up commit D153776 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

Szelethus added a comment.

This is the quintessential example of a patch where while the test files look 
promising, we need some evaluation on the real world. I understand that its a 
chore -- but this is simply the nature of the beast.

While the changes in the test file look promising. I'd be more pleased if some 
real world data would back it up (I understand that we shared a link around 
internally, but that kind of defeats the purpose of an open source review).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy accepted this revision.
donat.nagy added a comment.
This revision is now accepted and ready to land.

LGTM, but please wait until you can merge this together with D153776 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

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

Fixed review issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{'fclose' fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{'freopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{'fopen' fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,13 +107,16 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 clearerr(F);
 fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+// stdargs-note@-1 {{'fread' fails}}
 if (feof(F)) { // expected-note {{Taking true branch}}
   fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
   // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
@@ -115,10 +129,12 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' is successful}}
   if (feof(F)) { // expected-note {{Taking false branch}}
 fclose(F);
 return;
@@ -127,6 +143,7 @@
 return;
   }
   fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Thanks for the update! I have two minor remarks (string handling is complicated 
in C++...) but the code looks good otherwise.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305
+  std::string Note =
+  llvm::formatv(Case.getNote().str().c_str(),
+cast(Call.getDecl())->getDeclName());

Consider using the method `StringRef::data()` which directly converts a 
`StringRef` to a `const char *`. Your two-step conversion has the advantage 
that it adds a `\0` terminator even if the `StringRef` isn't null-terminated, 
but I cannot imagine a "natural" code change that would introduce references to 
non-null-terminated char arrays as note message templates.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
+  if (Node->succ_size() > 1)
+return Note.c_str();
+  return "";

The return type of this lambda is `std::string`, so I think it's pointless to 
convert `std::string Note` to a C string (which will be used to implicitly 
construct another `std::string`). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 535655.
balazske marked 3 inline comments as done.
balazske added a comment.

Fixed review issues.
Note tag is added for `fread`.
Notes contain now the function name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{'tmpfile' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{'fclose' fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{'freopen' is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{'fopen' is successful}}
+  // stdargs-note@-2 {{'fopen' is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{'fopen' fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,13 +107,16 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // expected-note {{Taking true branch}}
 clearerr(F);
 fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+// stdargs-note@-1 {{'fread' fails}}
 if (feof(F)) { // expected-note {{Taking true branch}}
   fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
   // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
@@ -115,10 +129,12 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{'fopen' is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
   fread(Buf, 1, 1, F);
+  // stdargs-note@-1 {{'fread' is successful}}
   if (feof(F)) { // expected-note {{Taking false branch}}
 fclose(F);
 return;
@@ -127,6 +143,7 @@
 return;
   }
   fread(Buf, 1, 1, F);   // expected-note {{Assuming stream reaches end-of-file here}}
+  // stdargs-note@-1 {{'fread' fails}}
   if (feof(F)) { // 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy requested changes to this revision.
donat.nagy added a comment.
This revision now requires changes to proceed.

As I started to review the code of the follow-up commit D153776 
, I noticed a dangling `StringRef` bug which 
belongs to this review.

Moreover, as a minor remark I'd note that LLVM has a dedicated class for 
storing string literal constants (btw I learned this from @Szelethus yesterday).




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1303
+  // use it as a note tag.
+  StringRef Note = Case.getNote();
+  if (Summary.getInvalidationKd() == EvalCallAsPure) {

This variable will be captured and stored by the lambda functions, so it should 
own the memory area of  the string.

In the current code this did not cause problems because this StringRef points 
to the memory area of a string literal (which stays valid); but if later 
changes introduced dynamically generated note tags, then this code would dump 
random memory garbage.

By the way, a very similar issue survived almost four years in 
CheckerContext.h, I'm fixing it in D153889.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1318
+  if (Node->succ_size() > 1)
+return Note.str();
+  return "";

Also update this when you change the type of `Note` to `std::string`.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1687-1688
 
+  const char *GenericSuccessMsg = "Assuming that the call is successful";
+  const char *GenericFailureMsg = "Assuming that the call fails";
+

Consider using the `StringLiteral` subclass of `StringRef` which is designed 
for this kind of application (and determines the length of the string in 
compile time instead of calling `strlen` during a runtime `const char * → 
StringRef` conversion):
https://llvm.org/doxygen/classllvm_1_1StringLiteral.html
(After the suggested change, also update the code that uses these variables!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

The source code changes LGTM. I'll soon check the results on the open source 
projects and give a final approval based on that.

By the way, I think this commit and the "display notes only if interesting" 
follow-up change (D153776 ) should be merged 
at the same time (but I'd guess that you already planned to do it that way).




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+// StdLibraryFunctionsChecker.
+ExplodedNode *Pred = const_cast(Node);
+if (!Case.getNote().empty()) {

balazske wrote:
> donat.nagy wrote:
> > balazske wrote:
> > > donat.nagy wrote:
> > > > Can you explain why is it safe to use `const_cast` here? (I don't see 
> > > > any concrete issue, but the engine has lots of invariants / unwritten 
> > > > rules and I fear that this might break one of them.)
> > > The node `Pred` should be modified only later when a successor is added 
> > > (`addTransition` has non-const parameter).
> > I understood that you //need// a non-const `ExplodedNode *` because 
> > `addTransition` expects it; I want to understand why you are //allowed to// 
> > `const_cast` it (why doesn't this confuse the engine logic).
> > 
> > Equivalent question from the other direction: Why did the author of 
> > `CheckerContext::getPredecessor()` specify that its return value is a 
> > //const// pointer to `ExplodedNode`?
> > 
> > If we can conclude that `const_cast` is valid in this kind of situation, 
> > then I'd also consider simply removing the "const" from the return type of 
> > `getPredecessor`.
> The `const_cast` is not needed at all if `Pred` and `Node` is made non-const, 
> and `getPredecessor` has a non-const version. The `Node` is saved because we 
> want to add transitions to it, it makes no sense to have it (a pointer to) 
> const. (Probably the const comes from a time when the `Node` was used only 
> for the lambda? In the lambda it could be const, if it matters.)
Sorry, it seems that I badly misread the source code of CheckerContext.h (I 
probably looked at the wrong line when I briefly jumped to the definition of 
`getPredecessor`). In fact, `getPredecessor` has only one version and it 
returns non-const `ExplodedNode *`.

This means that your code is (of course) completely valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+// StdLibraryFunctionsChecker.
+ExplodedNode *Pred = const_cast(Node);
+if (!Case.getNote().empty()) {

donat.nagy wrote:
> balazske wrote:
> > donat.nagy wrote:
> > > Can you explain why is it safe to use `const_cast` here? (I don't see any 
> > > concrete issue, but the engine has lots of invariants / unwritten rules 
> > > and I fear that this might break one of them.)
> > The node `Pred` should be modified only later when a successor is added 
> > (`addTransition` has non-const parameter).
> I understood that you //need// a non-const `ExplodedNode *` because 
> `addTransition` expects it; I want to understand why you are //allowed to// 
> `const_cast` it (why doesn't this confuse the engine logic).
> 
> Equivalent question from the other direction: Why did the author of 
> `CheckerContext::getPredecessor()` specify that its return value is a 
> //const// pointer to `ExplodedNode`?
> 
> If we can conclude that `const_cast` is valid in this kind of situation, then 
> I'd also consider simply removing the "const" from the return type of 
> `getPredecessor`.
The `const_cast` is not needed at all if `Pred` and `Node` is made non-const, 
and `getPredecessor` has a non-const version. The `Node` is saved because we 
want to add transitions to it, it makes no sense to have it (a pointer to) 
const. (Probably the const comes from a time when the `Node` was used only for 
the lambda? In the lambda it could be const, if it matters.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske updated this revision to Diff 534991.
balazske marked 5 inline comments as done.
balazske added a comment.

Fixed review issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{call is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{call fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
+  // stdargs-note@-2 {{call is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
+  // stdargs-note@-2 {{call is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{call fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,6 +107,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
@@ -115,6 +127,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
@@ -138,6 +151,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -10,7 +10,8 @@
 
 void check_fopen(void) {
   FILE *F = fopen("xxx", "r");
-  // expected-note@-1{{Assuming that function 'fopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fopen'}}
+  // expected-note@-2{{call is successful}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -22,7 +23,8 @@
 
 void check_tmpfile(void) {
   FILE *F = tmpfile();
-  // expected-note@-1{{Assuming that 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+// StdLibraryFunctionsChecker.
+ExplodedNode *Pred = const_cast(Node);
+if (!Case.getNote().empty()) {

balazske wrote:
> donat.nagy wrote:
> > Can you explain why is it safe to use `const_cast` here? (I don't see any 
> > concrete issue, but the engine has lots of invariants / unwritten rules and 
> > I fear that this might break one of them.)
> The node `Pred` should be modified only later when a successor is added 
> (`addTransition` has non-const parameter).
I understood that you //need// a non-const `ExplodedNode *` because 
`addTransition` expects it; I want to understand why you are //allowed to// 
`const_cast` it (why doesn't this confuse the engine logic).

Equivalent question from the other direction: Why did the author of 
`CheckerContext::getPredecessor()` specify that its return value is a //const// 
pointer to `ExplodedNode`?

If we can conclude that `const_cast` is valid in this kind of situation, then 
I'd also consider simply removing the "const" from the return type of 
`getPredecessor`.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309
+[Node, Note](PathSensitiveBugReport ) -> std::string {
+  if (Node->succ_size() > 1)
+return Note.str();

balazske wrote:
> donat.nagy wrote:
> > It's surprising to see this check inside the lambda, as its result does not 
> > depend on `BR`. My best guess is that it's performed here because the 
> > successors of `Node` will appear between the execution of the surrounding 
> > code and the execution of this lambda.
> > 
> > However, CheckerContext.h line 69-70 claims that "checkers should not 
> > retain the node in their state since the nodes might get invalidated." 
> > which would imply that the captured `Node` might be invalid when the lambda 
> > is called.
> This check is to decide if multiple cases could be applied, the same as if we 
> count how many times this place in the loop is executed (add a transition for 
> a case, constraints could be applied). This check is problematic because 
> other checkers can apply state splits before this checker is executed or 
> after it, in this way `StreamChecker` interferes with this code (it has a 
> state split for success/failure cases of same function, and here we see only 
> that a single case is applied on one branch). This is why this check is only 
> used in the `EvalCallAsPure` case (theoretically still another checker can 
> make a state split in PostCall before this where the same constraint is 
> applied, then the problem occurs again).
> 
> I made a solution that does not have this check but has 2 case loops instead, 
> but the mentioned problem (which exists when `if (Summary.getInvalidationKd() 
> == EvalCallAsPure)` is not used) did not go away. And it may not work to 
> search backwards for the first node with the same statement, because maybe 
> not the first one is where a state split is done.
> 
> I only think that if this lambda is called with the saved node, that node is 
> not invalid because it is part of a bug report call sequence.
> This check is problematic because [...details...]

Thanks for the explanation, now I see why this roundabout solution is needed.

> I only think that if this lambda is called with the saved node, that node is 
> not invalid because it is part of a bug report call sequence.

This is a good point, I think we can keep this "check successors in the lambda" 
solution. Please add a comment like "This check is performed inside the lambda, 
after other checkers had a chance to add other successors. Dereferencing the 
saved node object is valid because it's part of a bug report call sequence." to 
record the the reasoning that we discussed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+// StdLibraryFunctionsChecker.
+ExplodedNode *Pred = const_cast(Node);
+if (!Case.getNote().empty()) {

donat.nagy wrote:
> Can you explain why is it safe to use `const_cast` here? (I don't see any 
> concrete issue, but the engine has lots of invariants / unwritten rules and I 
> fear that this might break one of them.)
The node `Pred` should be modified only later when a successor is added 
(`addTransition` has non-const parameter).



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309
+[Node, Note](PathSensitiveBugReport ) -> std::string {
+  if (Node->succ_size() > 1)
+return Note.str();

donat.nagy wrote:
> It's surprising to see this check inside the lambda, as its result does not 
> depend on `BR`. My best guess is that it's performed here because the 
> successors of `Node` will appear between the execution of the surrounding 
> code and the execution of this lambda.
> 
> However, CheckerContext.h line 69-70 claims that "checkers should not retain 
> the node in their state since the nodes might get invalidated." which would 
> imply that the captured `Node` might be invalid when the lambda is called.
This check is to decide if multiple cases could be applied, the same as if we 
count how many times this place in the loop is executed (add a transition for a 
case, constraints could be applied). This check is problematic because other 
checkers can apply state splits before this checker is executed or after it, in 
this way `StreamChecker` interferes with this code (it has a state split for 
success/failure cases of same function, and here we see only that a single case 
is applied on one branch). This is why this check is only used in the 
`EvalCallAsPure` case (theoretically still another checker can make a state 
split in PostCall before this where the same constraint is applied, then the 
problem occurs again).

I made a solution that does not have this check but has 2 case loops instead, 
but the mentioned problem (which exists when `if (Summary.getInvalidationKd() 
== EvalCallAsPure)` is not used) did not go away. And it may not work to search 
backwards for the first node with the same statement, because maybe not the 
first one is where a state split is done.

I only think that if this lambda is called with the saved node, that node is 
not invalid because it is part of a bug report call sequence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309
+[Node, Note](PathSensitiveBugReport ) -> std::string {
+  if (Node->succ_size() > 1)
+return Note.str();

It's surprising to see this check inside the lambda, as its result does not 
depend on `BR`. My best guess is that it's performed here because the 
successors of `Node` will appear between the execution of the surrounding code 
and the execution of this lambda.

However, CheckerContext.h line 69-70 claims that "checkers should not retain 
the node in their state since the nodes might get invalidated." which would 
imply that the captured `Node` might be invalid when the lambda is called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

This is a good and useful commit; but I have some questions connected to the 
state transition handling code that you had to modify. (The State/ExplodedNode 
APIs of the engine are really counter-intuitive...  :( )




Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:316-319
+  C, (Twine("'errno'") +
+  Twine(" may be undefined after successful call to '") + Twine(Fn) +
+  Twine("'"))
  .str());

Consider using `llvm:formatv()` here and in other similar messages, it is much 
less verbose.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+// StdLibraryFunctionsChecker.
+ExplodedNode *Pred = const_cast(Node);
+if (!Case.getNote().empty()) {

Can you explain why is it safe to use `const_cast` here? (I don't see any 
concrete issue, but the engine has lots of invariants / unwritten rules and I 
fear that this might break one of them.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

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

Add note tag always if function is not evaluated as "pure".
Reformat the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -7,11 +7,13 @@
 
 void check_note_at_correct_open(void) {
   FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   FILE *F2 = tmpfile();
+  // stdargs-note@-1 {{call is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -20,6 +22,7 @@
   }
   rewind(F2);
   fclose(F2);
+  // stdargs-note@-1 {{call fails}}
   rewind(F1);
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
@@ -27,6 +30,7 @@
 
 void check_note_fopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -37,11 +41,13 @@
 
 void check_note_freopen(void) {
   FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
 return;
   F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+  // stdargs-note@-1 {{call is successful}}
   if (!F)
 // expected-note@-1 {{'F' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -52,6 +58,8 @@
 
 void check_note_leak_2(int c) {
   FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
+  // stdargs-note@-2 {{call is successful}}
   if (!F1)
 // expected-note@-1 {{'F1' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -59,6 +67,8 @@
 // expected-note@-4 {{Taking false branch}}
 return;
   FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  // stdargs-note@-1 {{call is successful}}
+  // stdargs-note@-2 {{call is successful}}
   if (!F2) {
 // expected-note@-1 {{'F2' is non-null}}
 // expected-note@-2 {{Taking false branch}}
@@ -84,6 +94,7 @@
 void check_track_null(void) {
   FILE *F;
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  // stdargs-note@-1 {{call fails}}
   if (F != NULL) {  // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
 fclose(F);
 return;
@@ -96,6 +107,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
@@ -115,6 +127,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   }
@@ -138,6 +151,7 @@
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
+  // stdargs-note@-1 {{call is successful}}
   if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
 return;
   int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -10,7 +10,8 @@
 
 void check_fopen(void) {
   FILE *F = fopen("xxx", "r");
-  // expected-note@-1{{Assuming that function 'fopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fopen'}}
+  // expected-note@-2{{call is successful}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -22,7 +23,8 @@
 
 void check_tmpfile(void) {
   FILE *F = tmpfile();
-  // 

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

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

balazske added inline comments.



Comment at: clang/test/Analysis/errno-stdlibraryfunctions-notes.c:17
   access("path", 0);
-  // expected-note@-1{{Assuming that function 'access' is successful, in this 
case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@-1{{Assuming that the call fails}}
+  access("path", 0);

This is the type of note that looks not necessary and even confusing. It could 
be any case of failure or success, the failure is chosen. This does not matter 
for the end result but can be confusing for users (one may think that there is 
a connection to the found bug).



Comment at: clang/test/Analysis/std-c-library-functions-path-notes.c:72
+return 0;
+  int l = islower(c);
+  f = fileno(f1); // \

Here no note is shown. Probably because the summary of `islower` has cases 
without note, these notes should be added.



Comment at: clang/test/Analysis/stream-errno-note.c:24
 void check_tmpfile(void) {
   FILE *F = tmpfile();
+  // expected-note@-1{{'errno' may be undefined after successful call to 
'tmpfile'}}

At this place a note 'Assuming that the call is successful' should be 
displayed. But this is not working because `StreamChecker` is enabled. 
`StreamChecker` makes a state split before `StdCLibraryFunctionsChecker` for 
`tmpfile` failure and success, then in `StdCLibraryFunctionsChecker` the 
successor count is 1 and the note is not added. Probably the logic can be 
improved by finding the first node that belongs to the `CallEvent`. Or count 
how many cases are applied before adding the note tags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in 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.

Change 1: ErrnoChecker notes show only messages related to errno,
not to assumption of success or failure of functions.
Change 2: StdLibraryFunctionsChecker adds its own note about success
or failure of functions, and the errno related note, independently.
Change 3: Every modeled function in StdLibraryFunctionsChecker
should have a note tag message in all "cases". This is not implemented yet,
only for file (stream) related functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153612

Files:
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/errno-stdlibraryfunctions-notes.c
  clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp
  clang/test/Analysis/std-c-library-functions-path-notes.c
  clang/test/Analysis/stream-errno-note.c

Index: clang/test/Analysis/stream-errno-note.c
===
--- clang/test/Analysis/stream-errno-note.c
+++ clang/test/Analysis/stream-errno-note.c
@@ -10,7 +10,7 @@
 
 void check_fopen(void) {
   FILE *F = fopen("xxx", "r");
-  // expected-note@-1{{Assuming that function 'fopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fopen'}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -22,7 +22,7 @@
 
 void check_tmpfile(void) {
   FILE *F = tmpfile();
-  // expected-note@-1{{Assuming that function 'tmpfile' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'tmpfile'}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -39,7 +39,7 @@
   if (!F)
 return;
   F = freopen("xxx", "w", F);
-  // expected-note@-1{{Assuming that function 'freopen' is successful}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'freopen'}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -56,7 +56,7 @@
   if (!F)
 return;
   (void)fclose(F);
-  // expected-note@-1{{Assuming that function 'fclose' is successful}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fclose'}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
 }
@@ -69,7 +69,7 @@
   if (!F)
 return;
   (void)fread(Buf, 1, 10, F);
-  // expected-note@-1{{Assuming that function 'fread' is successful}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
   (void)fclose(F);
@@ -83,7 +83,7 @@
   if (!F)
 return;
   int R = fwrite(Buf, 1, 10, F);
-  // expected-note@-1{{Assuming that function 'fwrite' is successful}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fwrite'}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
   (void)fclose(F);
@@ -96,7 +96,7 @@
   if (!F)
 return;
   (void)fseek(F, 11, SEEK_SET);
-  // expected-note@-1{{Assuming that function 'fseek' is successful}}
+  // expected-note@-1{{'errno' may be undefined after successful call to 'fseek'}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
   (void)fclose(F);
@@ -109,7 +109,7 @@
   if (!F)
 return;
   errno = 0;
-  rewind(F); // expected-note{{Function 'rewind' indicates failure only by setting of 'errno'}}
+  rewind(F); // expected-note{{'rewind' indicates failure only by setting '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'}}
 }
@@ -121,7 +121,8 @@
   if (!F)
 return;
   fileno(F);
-  // expected-note@-1{{Assuming that function 'fileno' is successful}}
+  // expected-note@-1{{Assuming that the call is successful}}
+  //