[clang][analyzer] Improve 'errno' handling in StdLibraryFunctionsChecker. (#71392)

The checker now displays one combined note tag for errno-related and
"case"-related notes. Previous functions in the errno-modeling part that
were used for construction of note tags are removed. The note tag added
by StdLibraryFunctionsChecker contains the code to display the note tag
for 'errno' (this was done previously by these removed functions).

5 files changed, 96 insertions, 85 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
index be2fa91..1b34ea0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
@@ -312,18 +312,6 @@ ProgramStateRef setErrnoStdMustBeChecked(ProgramStateRef State,
   return setErrnoState(State, MustBeChecked);
 }
 
-const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn) {
-  return getErrnoNoteTag(
-      C, llvm::formatv(
-             "'errno' may be undefined after successful call to '{0}'", Fn));
-}
-
-const NoteTag *getNoteTagForStdMustBeChecked(CheckerContext &C,
-                                             llvm::StringRef Fn) {
-  return getErrnoNoteTag(
-      C, llvm::formatv("'{0}' indicates failure only by setting 'errno'", Fn));
-}
-
 } // namespace errno_modeling
 } // namespace ento
 } // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
index 0707fd1..6b53572 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
+++ b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
@@ -84,8 +84,7 @@ const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message);
 
 /// Set errno state for the common case when a standard function is successful.
 /// Set \c ErrnoCheckState to \c MustNotBeChecked (the \c errno value is not
-/// affected). At the state transition a note tag created by
-/// \c getNoteTagForStdSuccess can be used.
+/// affected).
 ProgramStateRef setErrnoForStdSuccess(ProgramStateRef State, CheckerContext &C);
 
 /// Set errno state for the common case when a standard function fails.
@@ -100,23 +99,10 @@ ProgramStateRef setErrnoForStdFailure(ProgramStateRef State, CheckerContext &C,
 /// Set errno state for the common case when a standard function indicates
 /// failure only by \c errno. Sets \c ErrnoCheckState to \c MustBeChecked, and
 /// invalidates the errno region (clear of previous value).
-/// At the state transition a note tag created by
-/// \c getNoteTagForStdMustBeChecked can be used.
 /// \arg \c InvalE Expression that causes invalidation of \c errno.
 ProgramStateRef setErrnoStdMustBeChecked(ProgramStateRef State,
                                          CheckerContext &C, const Expr *InvalE);
 
-/// Generate the note tag that can be applied at the state generated by
-/// \c setErrnoForStdSuccess .
-/// \arg \c Fn Name of the (standard) function that is modeled.
-const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn);
-
-/// Generate the note tag that can be applied at the state generated by
-/// \c setErrnoStdMustBeChecked .
-/// \arg \c Fn Name of the (standard) function that is modeled.
-const NoteTag *getNoteTagForStdMustBeChecked(CheckerContext &C,
-                                             llvm::StringRef Fn);
-
 } // namespace errno_modeling
 } // namespace ento
 } // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 54a41b8..2163689 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -533,13 +533,11 @@ class StdLibraryFunctionsChecker
     virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                                   const Summary &Summary,
                                   CheckerContext &C) const = 0;
-    /// Get a NoteTag about the changes made to 'errno' and the possible bug.
-    /// It may return \c nullptr (if no bug report from \c ErrnoChecker is
-    /// expected).
-    virtual const NoteTag *describe(CheckerContext &C,
-                                    StringRef FunctionName) const {
-      return nullptr;
-    }
+    /// Get a description about what happens with 'errno' here and how it causes
+    /// a later bug report created by ErrnoChecker.
+    /// Empty return value means that 'errno' related bug may not happen from
+    /// the current analyzed function.
+    virtual const std::string describe(CheckerContext &C) const { return ""; }
 
     virtual ~ErrnoConstraintBase() {}
 
@@ -596,7 +594,8 @@ class StdLibraryFunctionsChecker
   };
 
   /// Set errno constraint at success cases of standard functions.
-  /// Success case: 'errno' is not allowed to be used.
+  /// Success case: 'errno' is not allowed to be used because the value is
+  /// undefined after successful call.
   /// \c ErrnoChecker can emit bug report after such a function call if errno
   /// is used.
   class SuccessErrnoConstraint : public ErrnoConstraintBase {
@@ -607,12 +606,15 @@ class StdLibraryFunctionsChecker
       return errno_modeling::setErrnoForStdSuccess(State, C);
     }
 
-    const NoteTag *describe(CheckerContext &C,
-                            StringRef FunctionName) const override {
-      return errno_modeling::getNoteTagForStdSuccess(C, FunctionName);
+    const std::string describe(CheckerContext &C) const {
+      return "'errno' becomes undefined after the call";
     }
   };
 
+  /// Set errno constraint at functions that indicate failure only with 'errno'.
+  /// In this case 'errno' is required to be observed.
+  /// \c ErrnoChecker can emit bug report after such a function call if errno
+  /// is overwritten without a read before.
   class ErrnoMustBeCheckedConstraint : public ErrnoConstraintBase {
   public:
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
@@ -622,9 +624,8 @@ class StdLibraryFunctionsChecker
                                                       Call.getOriginExpr());
     }
 
-    const NoteTag *describe(CheckerContext &C,
-                            StringRef FunctionName) const override {
-      return errno_modeling::getNoteTagForStdMustBeChecked(C, FunctionName);
+    const std::string describe(CheckerContext &C) const {
+      return "reading 'errno' is required to find out if the call has failed";
     }
   };
 
@@ -1392,17 +1393,28 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
     // Still add these note tags, the other checker should add only its
     // specialized note tags. These general note tags are handled always by
     // StdLibraryFunctionsChecker.
+
     ExplodedNode *Pred = Node;
-    if (!Case.getNote().empty()) {
-      const SVal RV = Call.getReturnValue();
-      // If there is a description for this execution branch (summary case),
-      // use it as a note tag.
-      std::string Note =
-          llvm::formatv(Case.getNote().str().c_str(),
-                        cast<NamedDecl>(Call.getDecl())->getDeclName());
-      if (Summary.getInvalidationKd() == EvalCallAsPure) {
+    DeclarationName FunctionName =
+        cast<NamedDecl>(Call.getDecl())->getDeclName();
+
+    std::string ErrnoNote = Case.getErrnoConstraint().describe(C);
+    std::string CaseNote;
+    if (Case.getNote().empty()) {
+      if (!ErrnoNote.empty())
+        ErrnoNote =
+            llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
+    } else {
+      CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
+    }
+    const SVal RV = Call.getReturnValue();
+
+    if (Summary.getInvalidationKd() == EvalCallAsPure) {
+      // Do not expect that errno is interesting (the "pure" functions do not
+      // affect it).
+      if (!CaseNote.empty()) {
         const NoteTag *Tag = C.getNoteTag(
-            [Node, Note, RV](PathSensitiveBugReport &BR) -> std::string {
+            [Node, CaseNote, RV](PathSensitiveBugReport &BR) -> std::string {
               // Try to omit the note if we know in advance which branch is
               // taken (this means, only one branch exists).
               // This check is performed inside the lambda, after other
@@ -1414,37 +1426,42 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
               // the successors). This is why this check is only used in the
               // EvalCallAsPure case.
               if (BR.isInteresting(RV) && Node->succ_size() > 1)
-                return Note;
+                return CaseNote;
               return "";
             });
         Pred = C.addTransition(NewState, Pred, Tag);
-      } else {
+      }
+    } else {
+      if (!CaseNote.empty() || !ErrnoNote.empty()) {
         const NoteTag *Tag =
-            C.getNoteTag([Note, RV](PathSensitiveBugReport &BR) -> std::string {
-              if (BR.isInteresting(RV))
-                return Note;
+            C.getNoteTag([CaseNote, ErrnoNote,
+                          RV](PathSensitiveBugReport &BR) -> std::string {
+              // If 'errno' is interesting, show the user a note about the case
+              // (what happened at the function call) and about how 'errno'
+              // causes the problem. ErrnoChecker sets the errno (but not RV) to
+              // interesting.
+              // If only the return value is interesting, show only the case
+              // note.
+              std::optional<Loc> ErrnoLoc =
+                  errno_modeling::getErrnoLoc(BR.getErrorNode()->getState());
+              bool ErrnoImportant = !ErrnoNote.empty() && ErrnoLoc &&
+                                    BR.isInteresting(ErrnoLoc->getAsRegion());
+              if (ErrnoImportant) {
+                BR.markNotInteresting(ErrnoLoc->getAsRegion());
+                if (CaseNote.empty())
+                  return ErrnoNote;
+                return llvm::formatv("{0}; {1}", CaseNote, ErrnoNote);
+              } else {
+                if (BR.isInteresting(RV))
+                  return CaseNote;
+              }
               return "";
             });
         Pred = C.addTransition(NewState, Pred, Tag);
       }
-
-      // Pred may be:
-      //  - a nullpointer, if we reach an already existing node (theoretically);
-      //  - a sink, when NewState is posteriorly overconstrained.
-      // In these situations we cannot add the errno note tag.
-      if (!Pred || Pred->isSink())
-        continue;
     }
 
-    // If we can get a note tag for the errno change, add this additionally to
-    // the previous. This note is only about value of 'errno' and is displayed
-    // if 'errno' is interesting.
-    if (const auto *D = dyn_cast<FunctionDecl>(Call.getDecl()))
-      if (const NoteTag *NT =
-              Case.getErrnoConstraint().describe(C, D->getNameAsString()))
-        Pred = C.addTransition(NewState, Pred, NT);
-
-    // Add the transition if no note tag could be added.
+    // Add the transition if no note tag was added.
     if (Pred == Node && NewState != State)
       C.addTransition(NewState);
   }
@@ -2038,7 +2055,8 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                 ErrnoMustNotBeChecked, GenericSuccessMsg)
           .Case({ArgumentCondition(1U, WithinRange, SingleValue(0)),
                  ReturnValueCondition(WithinRange, SingleValue(0))},
-                ErrnoMustNotBeChecked, GenericSuccessMsg)
+                ErrnoMustNotBeChecked,
+                "Assuming that argument 'size' to '{0}' is 0")
           .ArgConstraint(NotNullBuffer(ArgNo(0), ArgNo(1), ArgNo(2)))
           .ArgConstraint(NotNull(ArgNo(3)))
           .ArgConstraint(BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1),
diff --git a/clang/test/Analysis/errno-stdlibraryfunctions-notes.c b/clang/test/Analysis/errno-stdlibraryfunctions-notes.c
index c3fac58..41f5466 100644
--- a/clang/test/Analysis/errno-stdlibraryfunctions-notes.c
+++ b/clang/test/Analysis/errno-stdlibraryfunctions-notes.c
@@ -15,7 +15,7 @@ void clang_analyzer_warnIfReached();
 void test1() {
   access("path", 0);
   access("path", 0);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'access'}}
+  // expected-note@-1{{Assuming that 'access' is successful; 'errno' becomes undefined after the call}}
   if (errno != 0) {
     // expected-warning@-1{{An undefined value may be read from 'errno'}}
     // expected-note@-2{{An undefined value may be read from 'errno'}}
@@ -39,7 +39,7 @@ void test2() {
 void test3() {
   if (access("path", 0) != -1) {
     // Success path.
-    // expected-note@-2{{'errno' may be undefined after successful call to 'access'}}
+    // expected-note@-2{{Assuming that 'access' is successful; 'errno' becomes undefined after the call}}
     // expected-note@-3{{Taking true branch}}
     if (errno != 0) {
       // expected-warning@-1{{An undefined value may be read from 'errno'}}
diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c
index 32d9d4fd..d260eb2 100644
--- a/clang/test/Analysis/stream-errno-note.c
+++ b/clang/test/Analysis/stream-errno-note.c
@@ -10,7 +10,7 @@
 
 void check_fopen(void) {
   FILE *F = fopen("xxx", "r");
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fopen'}}
+  // expected-note@-1{{Assuming that 'fopen' is successful; 'errno' becomes undefined after the call}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -22,7 +22,7 @@ void check_fopen(void) {
 
 void check_tmpfile(void) {
   FILE *F = tmpfile();
-  // expected-note@-1{{'errno' may be undefined after successful call to 'tmpfile'}}
+  // expected-note@-1{{Assuming that 'tmpfile' is successful; 'errno' becomes undefined after the call}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -39,7 +39,7 @@ void check_freopen(void) {
   if (!F)
     return;
   F = freopen("xxx", "w", F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'freopen'}}
+  // expected-note@-1{{Assuming that 'freopen' is successful; 'errno' becomes undefined after the call}}
   // expected-note@+2{{'F' is non-null}}
   // expected-note@+1{{Taking false branch}}
   if (!F)
@@ -56,7 +56,7 @@ void check_fclose(void) {
   if (!F)
     return;
   (void)fclose(F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fclose'}}
+  // expected-note@-1{{Assuming that 'fclose' is successful; 'errno' becomes undefined after the call}}
   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 @@ void check_fread(void) {
   if (!F)
     return;
   (void)fread(Buf, 1, 10, F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}}
+  // expected-note@-1{{Assuming that 'fread' is successful; 'errno' becomes undefined after the call}}
   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 @@ void check_fread_size0(void) {
   if (!F)
     return;
   fread(Buf, 0, 1, F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}}
+  // expected-note@-1{{Assuming that argument 'size' to 'fread' is 0; 'errno' becomes undefined after the call}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
 }
@@ -96,7 +96,7 @@ void check_fread_nmemb0(void) {
   if (!F)
     return;
   fread(Buf, 1, 0, F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}}
+  // expected-note@-1{{Assuming that 'fread' is successful; 'errno' becomes undefined after the call}}
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
   // expected-note@-1{{An undefined value may be read from 'errno'}}
 }
@@ -109,7 +109,7 @@ void check_fwrite(void) {
   if (!F)
     return;
   int R = fwrite(Buf, 1, 10, F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fwrite'}}
+  // expected-note@-1{{Assuming that 'fwrite' is successful; 'errno' becomes undefined after the call}}
   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);
@@ -122,7 +122,7 @@ void check_fseek(void) {
   if (!F)
     return;
   (void)fseek(F, 11, SEEK_SET);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fseek'}}
+  // expected-note@-1{{Assuming that 'fseek' is successful; 'errno' becomes undefined after the call}}
   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);
@@ -135,7 +135,7 @@ void check_rewind_errnocheck(void) {
   if (!F)
     return;
   errno = 0;
-  rewind(F); // expected-note{{'rewind' indicates failure only by setting 'errno'}}
+  rewind(F); // expected-note{{After calling 'rewind' reading 'errno' is required to find out if the call has failed}}
   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'}}
 }
@@ -147,8 +147,27 @@ void check_fileno(void) {
   if (!F)
     return;
   fileno(F);
-  // expected-note@-1{{'errno' may be undefined after successful call to 'fileno'}}
+  // expected-note@-1{{Assuming that 'fileno' is successful; 'errno' becomes undefined after the call}}
   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);
 }
+
+void check_fwrite_zeroarg(size_t Siz) {
+  char Buf[] = "0123456789";
+  FILE *F = tmpfile();
+  // expected-note@+2{{'F' is non-null}}
+  // expected-note@+1{{Taking false branch}}
+  if (!F)
+    return;
+  errno = 0;
+  int R = fwrite(Buf, Siz, 1, F);
+  // expected-note@-1{{Assuming that argument 'size' to 'fwrite' is 0; 'errno' becomes undefined after the call}}
+  // expected-note@+2{{'R' is <= 0}}
+  // expected-note@+1{{Taking true branch}}
+  if (R <= 0) {
+    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);
+}