Skip to content

release/21.x: Thread safety analysis: Don't warn on acquiring reentrant capability (#150857) #151889

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

Merged
merged 1 commit into from
Aug 5, 2025

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 3, 2025

Backport a048aeb

Requested by: @aaronpuchert

@llvmbot
Copy link
Member Author

llvmbot commented Aug 3, 2025

@melver What do you think about merging this PR to the release branch?

@llvmbot llvmbot requested a review from melver August 3, 2025 21:34
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:analysis labels Aug 3, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Aug 3, 2025

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (llvmbot)

Changes

Backport a048aeb

Requested by: @aaronpuchert


Full diff: https://github.com/llvm/llvm-project/pull/151889.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/ThreadSafety.cpp (+1-1)
  • (modified) clang/test/SemaCXX/warn-thread-safety-negative.cpp (+32)
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 80e7c8eff671a..dadb0b757a2c8 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1331,7 +1331,7 @@ void ThreadSafetyAnalyzer::addLock(FactSet &FSet,
       FSet.removeLock(FactMan, NegC);
     }
     else {
-      if (inCurrentScope(*Entry) && !Entry->asserted())
+      if (inCurrentScope(*Entry) && !Entry->asserted() && !Entry->reentrant())
         Handler.handleNegativeNotHeld(Entry->getKind(), Entry->toString(),
                                       NegC.toString(), Entry->loc());
     }
diff --git a/clang/test/SemaCXX/warn-thread-safety-negative.cpp b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
index 9eabd67e4fc76..0caf6d6139e58 100644
--- a/clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -21,6 +21,15 @@ class LOCKABLE Mutex {
   void AssertReaderHeld() ASSERT_SHARED_LOCK();
 };
 
+class LOCKABLE REENTRANT_CAPABILITY ReentrantMutex {
+public:
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  // for negative capabilities
+  const ReentrantMutex& operator!() const { return *this; }
+};
+
 class SCOPED_LOCKABLE MutexLock {
 public:
   MutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
@@ -89,6 +98,29 @@ class Foo {
   }
 };
 
+class Reentrant {
+  ReentrantMutex mu;
+
+public:
+  void acquire() {
+    mu.Lock();   // no warning -- reentrant mutex
+    mu.Unlock();
+  }
+
+  void requireNegative() EXCLUSIVE_LOCKS_REQUIRED(!mu) { // warning?
+    mu.Lock();
+    mu.Unlock();
+  }
+
+  void callRequireNegative() {
+    requireNegative(); // expected-warning{{calling function 'requireNegative' requires negative capability '!mu'}}
+  }
+
+  void callHaveNegative() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
+    requireNegative();
+  }
+};
+
 }  // end namespace SimpleTest
 
 Mutex globalMutex;

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status Aug 4, 2025
…lvm#150857)

The point of reentrant capabilities is that they can be acquired
multiple times, so they should probably be excluded from requiring a
negative capability on acquisition via -Wthread-safety-negative.

However, we still propagate explicit negative requirements.

(cherry picked from commit a048aeb)
@tru tru merged commit d4046ae into llvm:release/21.x Aug 5, 2025
@github-project-automation github-project-automation bot moved this from Needs Merge to Done in LLVM Release Status Aug 5, 2025
Copy link

github-actions bot commented Aug 5, 2025

@aaronpuchert (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang Clang issues not falling into any other category
Projects
Development

Successfully merging this pull request may close these issues.

4 participants