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

[via cfe-commits]

================
@@ -1418,31 +1423,42 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
               return "";
             });
         Pred = C.addTransition(NewState, Pred, Tag);
-      } else {
-        const NoteTag *Tag =
-            C.getNoteTag([Note, RV](PathSensitiveBugReport &BR) -> std::string {
-              if (BR.isInteresting(RV))
-                return Note;
+      }
+    } else {
+      if (!Note.empty() || !ErrnoNote.empty()) {
+        const NoteTag *Tag = C.getNoteTag(
+            [Note, 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 (Note.empty())
+                  return ErrnoNote;
+                return llvm::formatv("{0}; {1}", Note, ErrnoNote);
+              } else {
+                if (BR.isInteresting(RV))
+                  return Note;
+              }
               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);
+    // 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;
----------------
DonatNagyE wrote:

```suggestion
```
This check can be removed; it was only needed to satisfy the preconditions of the `C.addTransition(NewState, Pred, NT);` call when the code applied the note tag for the errno change as a second separate transition starting in `Pred`.

The next if only operates in the trivial case when `Pred` still holds the unchanged initial `Node` (which is clearly non-null and AFAIK non-sink) and it also doesn't try to build an explicit transition onto `Pred`. (Note that `Pred` is declared in the middle of the loop, it doesn't survive into the next iteration.)

For the sake of ~~paranoia~~ safety, test the behavior of the code on some open source projects, but I don't see any reasons how this could introduce issues.

https://github.com/llvm/llvm-project/pull/71392