Skip to content

Thread Safety Analysis: Check managed capabilities of returned scoped capability #131831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

malek203
Copy link
Contributor

Verify that the return value of type scoped lockable manages the mutexes expected by the function annotations.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:analysis labels Mar 18, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2025

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Malek Ben Slimane (malek203)

Changes

Verify that the return value of type scoped lockable manages the mutexes expected by the function annotations.


Patch is 27.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/131831.diff

7 Files Affected:

  • (modified) clang/docs/ThreadSafetyAnalysis.rst (+5-4)
  • (modified) clang/include/clang/Analysis/Analyses/ThreadSafety.h (+8-4)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-2)
  • (modified) clang/lib/Analysis/ThreadSafety.cpp (+99-17)
  • (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+12-11)
  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+17-18)
  • (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+61-36)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..448c0670a4bf8 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -216,13 +216,14 @@ the scoped capability manages the specified capabilities in the given order.
 
   void require(MutexLocker& scope REQUIRES(mu1)) {
     scope.Unlock();
-    a = 0; // Warning!  Requires mu1.
+    a=0; // Warning!  Requires mu1.
     scope.Lock();
   }
 
   void testParameter() {
-    MutexLocker scope(&mu1), scope2(&mu2);
-    require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead of 'mu1'
+    MutexLocker scope(&mu1);
+    MutexLocker scope2(&mu2);
+    require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 'mu1'
     require(scope); // OK.
     scope.Unlock();
     require(scope); // Warning!  Requires mu1.
@@ -336,7 +337,7 @@ the scoped capability manages the specified capabilities in the given order.
     mu.Unlock();
   }
 
-  void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)) {
+  void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)){
     scope.Unlock(); // Warning! mu is not locked.
     scope.Lock();
   } // Warning! mu still held at the end of function.
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 20b75c46593e0..55fbb6cab8bde 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -243,10 +243,12 @@ class ThreadSafetyHandler {
   /// \param Kind -- The kind of the expected mutex.
   /// \param Expected -- The name of the expected mutex.
   /// \param Actual -- The name of the actual mutex.
+  /// \param ForParam -- Indicates whether the note applies to a function parameter.
   virtual void handleUnmatchedUnderlyingMutexes(SourceLocation Loc,
                                                 SourceLocation DLoc,
                                                 Name ScopeName, StringRef Kind,
-                                                Name Expected, Name Actual) {}
+                                                Name Expected, Name Actual,
+                                                bool ForParam) {}
 
   /// Warn when we get fewer underlying mutexes than expected.
   /// \param Loc -- The location of the call expression.
@@ -254,10 +256,11 @@ class ThreadSafetyHandler {
   /// \param ScopeName -- The name of the scope passed to the function.
   /// \param Kind -- The kind of the expected mutex.
   /// \param Expected -- The name of the expected mutex.
+  /// \param ForParam -- Indicates whether the note applies to a function parameter.
   virtual void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
                                                  SourceLocation DLoc,
                                                  Name ScopeName, StringRef Kind,
-                                                 Name Expected) {}
+                                                 Name Expected, bool ForParam) {}
 
   /// Warn when we get more underlying mutexes than expected.
   /// \param Loc -- The location of the call expression.
@@ -265,11 +268,12 @@ class ThreadSafetyHandler {
   /// \param ScopeName -- The name of the scope passed to the function.
   /// \param Kind -- The kind of the actual mutex.
   /// \param Actual -- The name of the actual mutex.
+  /// \param ForParam -- Indicates whether the note applies to a function parameter.
   virtual void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
                                                   SourceLocation DLoc,
                                                   Name ScopeName,
-                                                  StringRef Kind, Name Actual) {
-  }
+                                                  StringRef Kind, Name Actual,
+                                                  bool ForParam) {}
 
   /// Warn that L1 cannot be acquired before L2.
   virtual void handleLockAcquiredBefore(StringRef Kind, Name L1Name,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 627bebb31fc8d..82d9cb02960dd 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4117,8 +4117,8 @@ def warn_expect_more_underlying_mutexes : Warning<
 def warn_expect_fewer_underlying_mutexes : Warning<
   "did not expect %0 '%2' to be managed by '%1'">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
-def note_managed_mismatch_here_for_param : Note<
-  "see attribute on parameter here">;
+def note_managed_mismatch_here : Note<
+  "see attribute on %select{function|parameter}0 here">;
 
 
 // Thread safety warnings negative capabilities
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 6b5b49377fa08..2d64fd6d1696b 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -126,8 +126,8 @@ class FactEntry : public CapabilityExpr {
   SourceLocation AcquireLoc;
 
 public:
-  FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK,
-            SourceLocation Loc, SourceKind Src)
+  FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK, SourceLocation Loc,
+            SourceKind Src)
       : CapabilityExpr(CE), Kind(FK), LKind(LK), Source(Src), AcquireLoc(Loc) {}
   virtual ~FactEntry() = default;
 
@@ -1040,6 +1040,7 @@ class ThreadSafetyAnalyzer {
   std::vector<CFGBlockInfo> BlockInfo;
 
   BeforeSet *GlobalBeforeSet;
+  CapExprSet ExpectedReturnedCapabilities;
 
 public:
   ThreadSafetyAnalyzer(ThreadSafetyHandler &H, BeforeSet* Bset)
@@ -1969,7 +1970,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
     else
       llvm_unreachable("Unknown call kind");
   }
-  const auto *CalledFunction = dyn_cast<FunctionDecl>(D);
+  const FunctionDecl *CalledFunction = dyn_cast<FunctionDecl>(D);
   if (CalledFunction && Args.has_value()) {
     for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
       CapExprSet DeclaredLocks;
@@ -1980,7 +1981,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
           Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
                                               : ExclusiveLocksToAdd,
                                 A, Exp, D, Self);
-          Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
+          Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
           break;
         }
 
@@ -1992,7 +1993,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
             Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
           else
             Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
-          Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
+          Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
           break;
         }
 
@@ -2002,7 +2003,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
             Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
                                          A->isShared() ? AK_Read : AK_Written,
                                          Arg, POK_FunctionCall, Self, Loc);
-          Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
+          Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
           break;
         }
 
@@ -2010,7 +2011,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
           const auto *A = cast<LocksExcludedAttr>(At);
           for (auto *Arg : A->args())
             Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
-          Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
+          Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
           break;
         }
 
@@ -2018,15 +2019,14 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
           break;
         }
       }
-      if (DeclaredLocks.empty())
+      if (DeclaredLocks.size() == 0)
         continue;
-      CapabilityExpr Cp(Analyzer->SxBuilder.translate(Arg, nullptr),
-                        StringRef("mutex"), false);
+      CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(Arg, nullptr);
       if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(Arg->IgnoreCasts());
           Cp.isInvalid() && CBTE) {
         if (auto Object = Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
             Object != Analyzer->ConstructedObjects.end())
-          Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
+          Cp = CapabilityExpr(Object->second, StringRef(), false);
       }
       const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
       if (!Fact) {
@@ -2035,21 +2035,23 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
                                              Exp->getExprLoc());
         continue;
       }
-      const auto *Scope = cast<ScopedLockableFactEntry>(Fact);
+      const auto *Scope =
+          cast<ScopedLockableFactEntry>(Fact);
       for (const auto &[a, b] :
            zip_longest(DeclaredLocks, Scope->getUnderlyingMutexes())) {
         if (!a.has_value()) {
           Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
               Exp->getExprLoc(), D->getLocation(), Scope->toString(),
-              b.value().getKind(), b.value().toString());
+              b.value().getKind(), b.value().toString(), true);
         } else if (!b.has_value()) {
           Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
               Exp->getExprLoc(), D->getLocation(), Scope->toString(),
-              a.value().getKind(), a.value().toString());
-        } else if (!a.value().equals(b.value())) {
+              a.value().getKind(), a.value().toString(), true);
+        } else if (!a.value().matches(b.value())) {
           Analyzer->Handler.handleUnmatchedUnderlyingMutexes(
               Exp->getExprLoc(), D->getLocation(), Scope->toString(),
-              a.value().getKind(), a.value().toString(), b.value().toString());
+              a.value().getKind(), a.value().toString(), b.value().toString(),
+              true);
           break;
         }
       }
@@ -2294,6 +2296,25 @@ void BuildLockset::VisitMaterializeTemporaryExpr(
   }
 }
 
+static bool checkRecordTypeForScopedCapability(QualType Ty) {
+  const RecordType *RT = Ty->getAs<RecordType>();
+
+  if (!RT)
+    return false;
+
+  if (RT->getDecl()->hasAttr<ScopedLockableAttr>())
+    return true;
+
+  // Else check if any base classes have the attribute.
+  if (const auto *CRD = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
+    if (!CRD->forallBases([](const CXXRecordDecl *Base) {
+          return !Base->hasAttr<ScopedLockableAttr>();
+        }))
+      return true;
+  }
+  return false;
+}
+
 void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
   if (Analyzer->CurrentFunction == nullptr)
     return;
@@ -2316,6 +2337,49 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
         ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
         POK_ReturnPointer);
   }
+
+  if (!checkRecordTypeForScopedCapability(ReturnType))
+    return;
+
+  if (const auto *CBTE = dyn_cast<ExprWithCleanups>(RetVal))
+    RetVal = CBTE->getSubExpr();
+  RetVal = RetVal->IgnoreCasts();
+  if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(RetVal))
+    RetVal = CBTE->getSubExpr();
+  CapabilityExpr Cp;
+  if (auto Object = Analyzer->ConstructedObjects.find(RetVal);
+      Object != Analyzer->ConstructedObjects.end()) {
+    Cp = CapabilityExpr(Object->second, StringRef(), false);
+    Analyzer->ConstructedObjects.erase(Object);
+  }
+  if (!Cp.shouldIgnore()) {
+    const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
+    if (const ScopedLockableFactEntry *Scope =
+            cast_or_null<ScopedLockableFactEntry>(Fact)) {
+      CapExprSet LocksInReturnVal = Scope->getUnderlyingMutexes();
+      for (const auto &[a, b] : zip_longest(
+               Analyzer->ExpectedReturnedCapabilities, LocksInReturnVal)) {
+        if (!a.has_value()) {
+          Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
+              RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
+              Scope->toString(), b.value().getKind(), b.value().toString(),
+              false);
+        } else if (!b.has_value()) {
+          Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
+              RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
+              Scope->toString(), a.value().getKind(), a.value().toString(),
+              false);
+          break;
+        } else if (!a.value().matches(b.value())) {
+          Analyzer->Handler.handleUnmatchedUnderlyingMutexes(
+              RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
+              Scope->toString(), a.value().getKind(), a.value().toString(),
+              b.value().toString(), false);
+          break;
+        }
+      }
+    }
+  }
 }
 
 /// Given two facts merging on a join point, possibly warn and decide whether to
@@ -2480,11 +2544,22 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
     CapExprSet SharedLocksToAdd;
 
     SourceLocation Loc = D->getLocation();
+    bool ReturnsScopedCapability;
+    if (CurrentFunction)
+      ReturnsScopedCapability = checkRecordTypeForScopedCapability(
+          CurrentFunction->getReturnType().getCanonicalType());
+    else if (auto CurrentMethod = dyn_cast<ObjCMethodDecl>(D))
+      ReturnsScopedCapability = checkRecordTypeForScopedCapability(
+          CurrentMethod->getReturnType().getCanonicalType());
+    else
+      llvm_unreachable("Unknown function kind");
     for (const auto *Attr : D->attrs()) {
       Loc = Attr->getLocation();
       if (const auto *A = dyn_cast<RequiresCapabilityAttr>(Attr)) {
         getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
                     nullptr, D);
+        if (ReturnsScopedCapability)
+          getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
       } else if (const auto *A = dyn_cast<ReleaseCapabilityAttr>(Attr)) {
         // UNLOCK_FUNCTION() is used to hide the underlying lock implementation.
         // We must ignore such methods.
@@ -2493,12 +2568,19 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
         getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
                     nullptr, D);
         getMutexIDs(LocksReleased, A, nullptr, D);
+        if (ReturnsScopedCapability)
+          getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
       } else if (const auto *A = dyn_cast<AcquireCapabilityAttr>(Attr)) {
         if (A->args_size() == 0)
           return;
         getMutexIDs(A->isShared() ? SharedLocksAcquired
                                   : ExclusiveLocksAcquired,
                     A, nullptr, D);
+        if (ReturnsScopedCapability)
+          getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
+      } else if (const auto *A = dyn_cast<LocksExcludedAttr>(Attr)) {
+        if (ReturnsScopedCapability)
+          getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
       } else if (isa<ExclusiveTrylockFunctionAttr>(Attr)) {
         // Don't try to check trylock functions for now.
         return;
@@ -2541,7 +2623,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
       }
       if (UnderlyingLocks.empty())
         continue;
-      CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(), false);
+      CapabilityExpr Cp (SxBuilder.createVariable(Param), StringRef(), false);
       auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
           Cp, Param->getLocation(), FactEntry::Declared);
       for (const CapabilityExpr &M : UnderlyingLocks)
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 3d6da4f70f99e..da9151893c262 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1799,11 +1799,11 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
                : getNotes();
   }
 
-  OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
+  OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc, bool forParam) {
     return DeclLoc.isValid()
                ? getNotes(PartialDiagnosticAt(
-                     DeclLoc,
-                     S.PDiag(diag::note_managed_mismatch_here_for_param)))
+                     DeclLoc, S.PDiag(diag::note_managed_mismatch_here)
+                                  << forParam))
                : getNotes();
   }
 
@@ -1829,34 +1829,35 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
 
   void handleUnmatchedUnderlyingMutexes(SourceLocation Loc, SourceLocation DLoc,
                                         Name scopeName, StringRef Kind,
-                                        Name expected, Name actual) override {
+                                        Name expected, Name actual,
+                                        bool forParam) override {
     PartialDiagnosticAt Warning(Loc,
                                 S.PDiag(diag::warn_unmatched_underlying_mutexes)
                                     << Kind << scopeName << expected << actual);
     Warnings.emplace_back(std::move(Warning),
-                          makeManagedMismatchNoteForParam(DLoc));
+                          makeManagedMismatchNote(DLoc, forParam));
   }
 
   void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
                                          SourceLocation DLoc, Name scopeName,
-                                         StringRef Kind,
-                                         Name expected) override {
+                                         StringRef Kind, Name expected,
+                                         bool forParam) override {
     PartialDiagnosticAt Warning(
         Loc, S.PDiag(diag::warn_expect_more_underlying_mutexes)
                  << Kind << scopeName << expected);
     Warnings.emplace_back(std::move(Warning),
-                          makeManagedMismatchNoteForParam(DLoc));
+                          makeManagedMismatchNote(DLoc, forParam));
   }
 
   void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
                                           SourceLocation DLoc, Name scopeName,
-                                          StringRef Kind,
-                                          Name actual) override {
+                                          StringRef Kind, Name actual,
+                                          bool forParam) override {
     PartialDiagnosticAt Warning(
         Loc, S.PDiag(diag::warn_expect_fewer_underlying_mutexes)
                  << Kind << scopeName << actual);
     Warnings.emplace_back(std::move(Warning),
-                          makeManagedMismatchNoteForParam(DLoc));
+                          makeManagedMismatchNote(DLoc, forParam));
   }
 
   void handleInvalidLockExp(SourceLocation Loc) override {
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 769f5316ed934..b49b9a540b3a8 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -430,14 +430,12 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
   }
 }
 
-static bool checkFunParamsAreScopedLockable(Sema &S,
-                                            const ParmVarDecl *ParamDecl,
+static bool checkFunParamsAreScopedLockable(Sema &S, const ParmVarDecl *ParamDecl,
                                             const ParsedAttr &AL) {
   QualType ParamType = ParamDecl->getType();
-  if (const auto *RefType = ParamType->getAs<ReferenceType>();
-      RefType &&
-      checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
-    return true;
+  if (const auto *RefType = ParamType->getAs<ReferenceType>())
+    if (checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
+      return true;
   S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_scoped_lockable_param)
       << AL;
   return false;
@@ -670,9 +668,10 @@ static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 }
 
 static void handleLocksExcludedAttr(...
[truncated]

Copy link

github-actions bot commented Mar 18, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

… capability

Verify that the return value of type scoped lockable manages the
mutexes expected by the function annotations.
@malek203 malek203 force-pushed the consistency-check-for-return-value branch from bf34707 to 7157d4c Compare March 18, 2025 16:26
@aaronpuchert aaronpuchert self-requested a review March 18, 2025 16:42
Copy link
Member

@aaronpuchert aaronpuchert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just some small nitpicks.

@@ -2041,15 +2042,16 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
if (!a.has_value()) {
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
b.value().getKind(), b.value().toString());
b.value().getKind(), b.value().toString(), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense to add comments. These can be checked.

Suggested change
b.value().getKind(), b.value().toString(), true);
b.value().getKind(), b.value().toString(), /*ForParam=*/true);

Same below.

a.value().getKind(), a.value().toString());
} else if (!a.value().equals(b.value())) {
a.value().getKind(), a.value().toString(), true);
} else if (!a.value().matches(b.value())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should remain equals.

Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
Scope->toString(), b.value().getKind(), b.value().toString(),
false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here: add comments on the parameter.

if (!a.has_value()) {
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
Scope->toString(), b.value().getKind(), b.value().toString(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can use the unchecked variants here:

Suggested change
Scope->toString(), b.value().getKind(), b.value().toString(),
Scope->toString(), b->getKind(), b->toString(),

Same below.

@@ -1799,11 +1799,11 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
: getNotes();
}

OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc, bool forParam) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc, bool forParam) {
OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc, bool ForParam) {

Also below.

RelockableMutexLock lockTest2() EXCLUSIVE_LOCK_FUNCTION(mu) {
return RelockableMutexLock(&mu, DeferTraits{});
} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
#endif

} // end namespace PassingScope
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these tests should be in the namespace ReturnScopedLockable?

// expected-note@+1{{see attribute on function here}}
DoubleMutexLock returnFewerTest() EXCLUSIVE_LOCK_FUNCTION(mu){
// expected-note@+1{{mutex acquired here}}
return DoubleMutexLock(&mu,&mu2); // expected-warning{{did not expect mutex 'mu2' to be managed by '<temporary>'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return DoubleMutexLock(&mu,&mu2); // expected-warning{{did not expect mutex 'mu2' to be managed by '<temporary>'}}
return DoubleMutexLock(&mu, &mu2); // expected-warning{{did not expect mutex 'mu2' to be managed by '<temporary>'}}

@@ -3566,6 +3565,38 @@ void releaseMemberCall() {
ReleasableMutexLock lock(&obj.mu);
releaseMember(obj, lock);
}
#ifdef __cpp_guaranteed_copy_elision
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add blank lines around #ifdef and #endif for readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants