update based on review advice

Author: flovent

clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

@@ -185,6 +185,9 @@ class BlockInCriticalSectionChecker
   const BugType BlockInCritSectionBugType{
       this, "Call to blocking function in critical section", "Blocking Error"};
 
+  using O_NONBLOCKValueTy = std::optional<int>;
+  mutable std::optional<O_NONBLOCKValueTy> O_NONBLOCKValue;
+
   void reportBlockInCritSection(const CallEvent &call, CheckerContext &C) const;
 
   [[nodiscard]] const NoteTag *createCritSectionNote(CritSectionMarker M,
@@ -317,17 +320,16 @@ void BlockInCriticalSectionChecker::handleUnlock(
 void BlockInCriticalSectionChecker::handleOpen(const CallEvent &Call,
                                                CheckerContext &C) const {
   const auto *Flag = Call.getArgExpr(1);
-  static std::optional<int> ValueOfONonBlockVFlag =
-      tryExpandAsInteger("O_NONBLOCK", C.getBugReporter().getPreprocessor());
-  if (!ValueOfONonBlockVFlag)
-    return;
-
   SVal FlagSV = C.getState()->getSVal(Flag, C.getLocationContext());
   const llvm::APSInt *FlagV = FlagSV.getAsInteger();
   if (!FlagV)
     return;
 
-  if ((*FlagV & ValueOfONonBlockVFlag.value()) != 0)
+  if (!O_NONBLOCKValue)
+    O_NONBLOCKValue =
+        tryExpandAsInteger("O_NONBLOCK", C.getBugReporter().getPreprocessor());
+
+  if (*O_NONBLOCKValue && ((*FlagV & **O_NONBLOCKValue) != 0))
     if (SymbolRef SR = Call.getReturnValue().getAsSymbol()) {
       C.addTransition(C.getState()->add<NonBlockFileDescriptor>(SR));
     }
@@ -348,18 +350,16 @@ void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
     // not cause block in these situations, don't report
     StringRef FuncName = Call.getCalleeIdentifier()->getName();
     if (FuncName == "read" || FuncName == "recv") {
-      const auto *Arg = Call.getArgExpr(0);
-      if (!Arg)
+      SVal SV = Call.getArgSVal(0);
+      SValBuilder &SVB = C.getSValBuilder();
+      ProgramStateRef state = C.getState();
+      ConditionTruthVal CTV =
+          state->areEqual(SV, SVB.makeIntVal(-1, C.getASTContext().IntTy));
+      if (CTV.isConstrainedTrue())
         return;
 
-      SVal SV = C.getSVal(Arg);
-      if (const auto *IntValue = SV.getAsInteger()) {
-        if (*IntValue == -1)
-          return;
-      }
-
-      SymbolRef SR = C.getSVal(Arg).getAsSymbol();
-      if (SR && C.getState()->contains<NonBlockFileDescriptor>(SR)) {
+      SymbolRef SR = SV.getAsSymbol();
+      if (SR && state->contains<NonBlockFileDescriptor>(SR)) {
         return;
       }
     }

clang/test/Analysis/Inputs/system-header-simulator-cxx-std-locks.h

@@ -0,0 +1,17 @@
+// This is a fake system header with divide-by-zero bugs introduced in
+// c++ std library functions. We use these bugs to test hard-coded
+// suppression of diagnostics within standard library functions that are known
+// to produce false positives.
+
+#pragma clang system_header
+namespace std {
+struct mutex {
+  void lock() {}
+  void unlock() {}
+};
+
+template <typename T> struct lock_guard {
+  lock_guard<T>(std::mutex) {}
+  ~lock_guard<T>() {}
+};
+} // namespace std

clang/test/Analysis/issue-124474.cpp

@@ -6,45 +6,35 @@
 
 // expected-no-diagnostics
 
-namespace std {
-  struct mutex {
-    void lock() {}
-    void unlock() {}
-  };
-  template<typename T>
-  struct lock_guard {
-    lock_guard<T>(std::mutex) {}
-    ~lock_guard<T>() {}
-  };
-  template<typename T>
-  struct unique_lock {
-    unique_lock<T>(std::mutex) {}
-    ~unique_lock<T>() {}
-  };
-  template<typename T>
-  struct not_real_lock {
-    not_real_lock<T>(std::mutex) {}
-  };
-  } // namespace std
+#include "Inputs/system-header-simulator-cxx-std-locks.h"
 
 std::mutex mtx;
 using ssize_t = long long;
 using size_t = unsigned long long;
-int open (const char *__file, int __oflag, ...);
+int open(const char *__file, int __oflag, ...);
 ssize_t read(int fd, void *buf, size_t count);
 void close(int fd);
-#define O_RDONLY	     00
-# define O_NONBLOCK	  04000
+#define O_RDONLY 00
+#define O_NONBLOCK 04000
 
-void foo()
-{
-    std::lock_guard<std::mutex> lock(mtx);
+void foo() {
+  std::lock_guard<std::mutex> lock(mtx);
 
-    const char *filename = "example.txt";
-    int fd = open(filename, O_RDONLY | O_NONBLOCK);
+  const char *filename = "example.txt";
+  int fd = open(filename, O_RDONLY | O_NONBLOCK);
 
-    char buffer[200] = {};
-    read(fd, buffer, 199); // no-warning: fd is a non-block file descriptor
-    close(fd);
+  char buffer[200] = {};
+  read(fd, buffer, 199); // no-warning: fd is a non-block file descriptor or equals to -1
+  close(fd);
 }
 
+void foo1(int fd) {
+  std::lock_guard<std::mutex> lock(mtx);
+
+  const char *filename = "example.txt";
+  char buffer[200] = {};
+  if (fd == -1)
+    read(fd, buffer, 199); // no-warning: consider file descriptor is a symbol equals to -1
+  close(fd);
+}
+