[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)

[Endre Fülöp via cfe-commits]

================
@@ -20,48 +20,180 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include <iterator>
+#include <utility>
+#include <variant>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+
+struct CritSectionMarker {
+  const Expr *LockExpr{};
+  const MemRegion *LockReg{};
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.Add(LockExpr);
+    ID.Add(LockReg);
+  }
+
+  [[nodiscard]] constexpr bool
+  operator==(const CritSectionMarker &Other) const noexcept {
+    return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+  }
+  [[nodiscard]] constexpr bool
+  operator!=(const CritSectionMarker &Other) const noexcept {
+    return !(*this == Other);
+  }
+};
+
+class FirstArgMutexDescriptor {
+  CallDescription LockFn;
+  CallDescription UnlockFn;
+
+public:
+  FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+      : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+  [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+    return LockFn.matches(Call) && Call.getNumArgs() > 0;
+  }
+  [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+    return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+  }
+  [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+    return Call.getArgSVal(0).getAsRegion();
+  }
+  [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+    return Call.getArgSVal(0).getAsRegion();
+  }
+};
+
+class MemberMutexDescriptor {
+  CallDescription LockFn;
+  CallDescription UnlockFn;
+
+public:
+  MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+      : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+  [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+    return LockFn.matches(Call);
+  }
+  bool matchesUnlock(const CallEvent &Call) const {
+    return UnlockFn.matches(Call);
+  }
+  [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+    return cast<CXXMemberCall>(Call).getCXXThisVal().getAsRegion();
+  }
+  [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+    return cast<CXXMemberCall>(Call).getCXXThisVal().getAsRegion();
+  }
+};
+
+class RAIIMutexDescriptor {
+  mutable const IdentifierInfo *Guard{};
+  mutable bool IdentifierInfoInitialized{};
+  mutable llvm::SmallString<32> GuardName{};
+
+  void initIdentifierInfo(const CallEvent &Call) const {
+    if (!IdentifierInfoInitialized) {
+      // In case of checking C code, or when the corresponding headers are not
+      // included, we might end up query the identifier table every time when
+      // this function is called instead of early returning it. To avoid this, a
+      // bool variable (IdentifierInfoInitialized) is used and the function will
+      // be run only once.
+      Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+          GuardName);
+      IdentifierInfoInitialized = true;
+    }
+  }
+
+public:
+  RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+  [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+    initIdentifierInfo(Call);
+    const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call);
+    if (!Ctor)
+      return false;
+    auto *IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+    return IdentifierInfo == Guard;
+  }
+  [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+    initIdentifierInfo(Call);
+    const auto *Dtor = dyn_cast<CXXDestructorCall>(&Call);
+    if (!Dtor)
+      return false;
+    auto *IdentifierInfo =
+        cast<CXXRecordDecl>(Dtor->getDecl()->getParent())->getIdentifier();
----------------
gamesh411 wrote:

The constructor and destructor cases are not symmetric; the getParent() for CXXConstructorDecl uses the cast internally, while the getParent() only gives DeclContext.
So, for the constructor, we have the following type chain:
`CXXConstructorCall` ---[ `getDecl()` ]---> `CXXConstructorDecl` ---[ `getParent()` ]---> `CXXRecordDecl`
and for the destructor we have the following:
`CXXDescructorCall` ---[ `getDecl()` ]---> `FunctionDecl` ---[ `getParent()` ]--> `DeclContext` (and this has to be cast to CXXRecordDecl)
The API is just not symmetric enough.

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