Thread Safety Analysis: Differentiate between lock sets at real join points and expected/actual sets at function end

[malek203]

This fixes false positives related to returning a scoped lockable object. At the end of a function, we check managed locks instead of scoped locks.

At real join points, we skip checking managed locks because we assume that the scope keeps track of its underlying mutexes and will release them at its destruction. So, checking for the scopes is sufficient. However, at the end of a function, we aim at comparing the expected and the actual lock sets. There, we skip checking scoped locks to prevent to get duplicate warnings for the same lock.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Malek Ben Slimane (malek203)

Changes

This fixes false positives related to returning a scoped lockable object. At the end of a function, we check managed locks instead of scoped locks.

At real join points, we skip checking managed locks because we assume that the scope keeps track of its underlying mutexes and will release them at its destruction. So, checking for the scopes is sufficient. However, at the end of a function, we aim at comparing the expected and the actual lock sets. There, we skip checking scoped locks to prevent to get duplicate warnings for the same lock.

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/ThreadSafety.cpp (+6-2)
• (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+8-12)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843c9bf83e..c4a83b069e0792 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -922,6 +922,9 @@ class ScopedLockableFactEntry : public FactEntry {
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
                                 ThreadSafetyHandler &Handler) const override {
+    if (LEK == LEK_LockedAtEndOfFunction || LEK == LEK_NotLockedAtEndOfFunction)
+      return;
+
     for (const auto &UnderlyingMutex : UnderlyingMutexes) {
       const auto *Entry = FSet.findLock(FactMan, UnderlyingMutex.Cap);
       if ((UnderlyingMutex.Kind == UCK_Acquired && Entry) ||
@@ -2224,7 +2227,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
       if (join(FactMan[*EntryIt], ExitFact,
                EntryLEK != LEK_LockedSomeLoopIterations))
         *EntryIt = Fact;
-    } else if (!ExitFact.managed()) {
+    } else if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) {
       ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
                                              EntryLEK, Handler);
     }
@@ -2236,7 +2239,8 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
     const FactEntry *ExitFact = ExitSet.findLock(FactMan, *EntryFact);
 
     if (!ExitFact) {
-      if (!EntryFact->managed() || ExitLEK == LEK_LockedSomeLoopIterations)
+      if (!EntryFact->managed() || ExitLEK == LEK_LockedSomeLoopIterations ||
+          ExitLEK == LEK_NotLockedAtEndOfFunction)
         EntryFact->handleRemovalFromIntersection(EntrySetOrig, FactMan, JoinLoc,
                                                  ExitLEK, Handler);
       if (ExitLEK == LEK_LockedSomePredecessors)
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index af9254508d8050..8477200456d985 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -6077,24 +6077,20 @@ namespace ReturnScopedLockable {
 class Object {
 public:
   MutexLock lock() EXCLUSIVE_LOCK_FUNCTION(mutex) {
-    // TODO: False positive because scoped lock isn't destructed.
-    return MutexLock(&mutex); // expected-note {{mutex acquired here}}
-  }                           // expected-warning {{mutex 'mutex' is still held at the end of function}}
+    return MutexLock(&mutex);
+  }
 
   ReaderMutexLock lockShared() SHARED_LOCK_FUNCTION(mutex) {
-    // TODO: False positive because scoped lock isn't destructed.
-    return ReaderMutexLock(&mutex); // expected-note {{mutex acquired here}}
-  }                                 // expected-warning {{mutex 'mutex' is still held at the end of function}}
+    return ReaderMutexLock(&mutex);
+  }
 
   MutexLock adopt() EXCLUSIVE_LOCKS_REQUIRED(mutex) {
-    // TODO: False positive because scoped lock isn't destructed.
-    return MutexLock(&mutex, true); // expected-note {{mutex acquired here}}
-  }                                 // expected-warning {{mutex 'mutex' is still held at the end of function}}
+    return MutexLock(&mutex, true);
+  }
 
   ReaderMutexLock adoptShared() SHARED_LOCKS_REQUIRED(mutex) {
-    // TODO: False positive because scoped lock isn't destructed.
-    return ReaderMutexLock(&mutex, true); // expected-note {{mutex acquired here}}
-  }                                       // expected-warning {{mutex 'mutex' is still held at the end of function}}
+    return ReaderMutexLock(&mutex, true);
+  }
 
   int x GUARDED_BY(mutex);
   void needsLock() EXCLUSIVE_LOCKS_REQUIRED(mutex);

[aaronpuchert]

It could be argued that the issue here is something different, namely that we don't add the scoped lock in the return slot to the expected function exit set. But this is likely not enough:

• We don't compare the set of underlying mutexes of a scope at (real) join points, and there is no very compelling reason to do that at the moment: scopes are assigned their underlying mutexes at the time of construction, and we don't support changing them throughout the lifetime of the scope. So if both branches in a join have the same scope object, it comes from the same declaration and thus also initialization. So it will manage the same set of mutexes regardless of where we come from.
• The status of a scoped capability roughly tracks its lifetime,¹ which makes it not so interesting by itself for the analysis. Because we only support local variables, the lifetime matches the extent of a lexical (block) scope. So we already know that some scope object is going to be returned, and that can only be the only scope object still alive at the end of the function.
• At some point we should introduce a check that the set of managed capabilities of the returned scope matches the set of capabilities mentioned in attributes on the function. Since @malek203 is working on something similar I think we might see this relatively soon. However, this seems orthogonal to checking the status of managed mutexes, because as mentioned we don't generally want to check them on join points, just at the end of a function. Maybe we'll go back to comparing scoped capabilities, but let's see. For now this seems like the right change, and it should be easy to revert should we decide to move the differences of join point handling into the scoped capability handling instead.

So this looks good to me, but let's wait if @AaronBallman has anything to add.

¹With exceptions: if they're constructed through a constructor without annotation we don't see the beginning of their lifetime, if the destructor has no annotation we don't see the end of it. But not having such annotations does not seem like a valid use case, and at some point we might change the behavior to strictly follow the lifetime of the object.

[github-actions]

@malek203 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[glandium]

Before this change the following (reduced) code didn't emit a warning, but now does:

class __attribute__((capability("mutex"))) StaticMutex {
 public:
  void Lock() __attribute__((exclusive_lock_function()))  { /* unimplemented */ }

  void Unlock() __attribute__((unlock_function())) { /* unimplemented */ }

  void AssertCurrentThreadOwns() __attribute__((assert_capability(this))) {
  }
};

class __attribute__((scoped_lockable)) StaticMutexAutoLock {
 public:
  explicit StaticMutexAutoLock(StaticMutex& aLock) __attribute__((exclusive_lock_function(aLock))) { /* unimplemented */ }

  ~StaticMutexAutoLock(void) __attribute__((unlock_function())) { /* unimplemented */ }
};

class __attribute__((scoped_lockable)) StaticMutexAutoUnlock {
 public:
  explicit StaticMutexAutoUnlock(StaticMutex& aLock) __attribute__((release_capability(aLock))) { /* unimplemented */ }

  ~StaticMutexAutoUnlock() __attribute__((release_capability())) { /* unimplemented */ }
};

StaticMutex sMutex;

bool InitPreferredSampleRate() {
  sMutex.AssertCurrentThreadOwns();
  {
    StaticMutexAutoUnlock unlock(sMutex);
  }
  return true;
}

Now it says warning: mutex 'sMutex' is still held at the end of function [-Wthread-safety-analysis] because apparently it's not "propagating" from the attribute on AssertCurrentThreadOwns?

[aaronpuchert]

The warning is arguably wrong, and this might even be considered a regression. However, operations on asserted capabilities have been an issue before this change, and this worked for scoped capabilities only accidentally. If you do this without a scoped capability, you get a warning even before this change:

bool InitPreferredSampleRateDirect() {
  sMutex.AssertCurrentThreadOwns();
  sMutex.Unlock();
  sMutex.Lock();
  return true;
} // warning: mutex 'sMutex' is still held at the end of function [-Wthread-safety-analysis]

This should behave exactly like your example, because it does the same operations in the same order. They are semantically equivalent. And if you remove sMutex.Lock(), there is no warning even though there should be one. Doing this via scoped capability hid the warning because we didn't look at the underlying mutexes of a scoped lock at the end of a function prior to this change, but that is arguably wrong. We should look at the set of underlying mutexes to find the bug here:

class __attribute__((scoped_lockable)) StaticMutexAdoptLock {
 public:
  explicit StaticMutexAdoptLock(StaticMutex& aLock) __attribute__((exclusive_locks_required(aLock)));

  ~StaticMutexAdoptLock(void) __attribute__((unlock_function()));
};

bool InitPreferredSampleRateAdopt() {
  sMutex.AssertCurrentThreadOwns();
  {
    StaticMutexAdoptLock unlock(sMutex);
  }
  return true;
} // should warn here, but we don't

Even after this change, we don't warn about the unscoped version of this, but this is because the modelling of asserted capabilities is wrong. Without this change and a fixed modelling of asserted capabilities, we still wouldn't warn, because sMutex is managed. That's why I think this change is correct even though it introduced a false positive.

I agree that the modelling of asserted capabilities should be fixed, but it's orthogonal to this change. For now, the only thing that's reliably supported with asserted capabilities is to leave them as they are. We need to figure out what the semantics are for locking asserted capabilities, perhaps through a scoped capability, and how we implement that. Currently, unlocking the asserted capability just removes it from the set of held capabilities, which is true but doesn't account for the fact that we should reacquire it. Similarly, reacquiring the asserted capability introduces an acquired capability, but the function wasn't declared to have the capability acquired at the end of the function. That's why you get the warning. Maybe we should treat this as asserted capability, because you're just reacquiring a previously released asserted capability. Asserted capabilities are fine to be held at the end of a function.