[clang][ThreadSafety] Handle mutex scope #157171

Prabhuk · 2025-09-05T20:36:46Z

Before emitting warning about locks being held at the end of function
scope check if the underlying mutex is function scoped or not.

Issue: #156760

llvmbot · 2025-09-05T20:37:18Z

@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: Prabhu Rajasekaran (Prabhuk)

Changes

Before emitting warning about locks being held at the end of function
scope check if the underlying mutex is function scoped or not.

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

1 Files Affected:

(modified) clang/lib/Analysis/ThreadSafety.cpp (+16)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 131170df9976e..b215a6e6d74cc 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2443,6 +2443,22 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
       if (join(FactMan[*EntryIt], ExitFact, JoinLoc, EntryLEK))
         *EntryIt = Fact;
     } else if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) {
+      if (EntryLEK == LEK_LockedAtEndOfFunction) {
+        const til::SExpr *Sexp = ExitFact.sexpr();
+        const VarDecl *Var = nullptr;
+
+        if (const auto *Proj = dyn_cast<til::Project>(Sexp)) {
+          if (const auto *Base = dyn_cast<til::LiteralPtr>(Proj->record()))
+            Var = dyn_cast_or_null<VarDecl>(Base->clangDecl());
+        } else if (const auto *LP = dyn_cast<til::LiteralPtr>(Sexp)) {
+          Var = dyn_cast_or_null<VarDecl>(LP->clangDecl());
+        }
+
+        if (Var && Var->getStorageDuration() == SD_Automatic &&
+            Var->getDeclContext() == CurrentFunction) {
+          continue;
+        }
+      }
       ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
                                              EntryLEK, Handler);
     }

llvmbot · 2025-09-05T20:37:18Z

@llvm/pr-subscribers-clang-analysis

Author: Prabhu Rajasekaran (Prabhuk)

Changes

Before emitting warning about locks being held at the end of function
scope check if the underlying mutex is function scoped or not.

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

1 Files Affected:

(modified) clang/lib/Analysis/ThreadSafety.cpp (+16)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 131170df9976e..b215a6e6d74cc 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2443,6 +2443,22 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
       if (join(FactMan[*EntryIt], ExitFact, JoinLoc, EntryLEK))
         *EntryIt = Fact;
     } else if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) {
+      if (EntryLEK == LEK_LockedAtEndOfFunction) {
+        const til::SExpr *Sexp = ExitFact.sexpr();
+        const VarDecl *Var = nullptr;
+
+        if (const auto *Proj = dyn_cast<til::Project>(Sexp)) {
+          if (const auto *Base = dyn_cast<til::LiteralPtr>(Proj->record()))
+            Var = dyn_cast_or_null<VarDecl>(Base->clangDecl());
+        } else if (const auto *LP = dyn_cast<til::LiteralPtr>(Sexp)) {
+          Var = dyn_cast_or_null<VarDecl>(LP->clangDecl());
+        }
+
+        if (Var && Var->getStorageDuration() == SD_Automatic &&
+            Var->getDeclContext() == CurrentFunction) {
+          continue;
+        }
+      }
       ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
                                              EntryLEK, Handler);
     }

Before emitting warning about locks being held at the end of function scope check if the underlying mutex is function scoped or not.

PiJoules

LGTM and can confirm this fixes #156760. Could you add that repro as a regression test here.

Prabhuk · 2025-09-05T22:13:05Z

LGTM and can confirm this fixes #156760. Could you add that repro as a regression test here.

Can you check if the regression tests added to clang/test/PCH/thread-safety-attrs.cpp covers the cases?

PiJoules · 2025-09-05T22:14:55Z

LGTM and can confirm this fixes #156760. Could you add that repro as a regression test here.

Can you check if the regression tests added to clang/test/PCH/thread-safety-attrs.cpp covers the cases?

That test passes mutexes to std::lock but the example in the bug has unique_locks passed to std::lock

ilovepi · 2025-09-05T23:16:55Z

Is there anything in this that requires pch? Seems like the other thread safety tests would make more sense.

Prabhuk · 2025-09-05T23:58:34Z

Is there anything in this that requires pch? Seems like the other thread safety tests would make more sense.

Extracted regression tests into clang/test/Analysis directory into a separate file. PTAL.

aaronpuchert

It looks like there is some confusion around the concepts here: unique_lock is not a lockable but a scoped_lockable. The latter should be automatically released if you annotate the destructor. The former should in my view be manually released before they're destroyed. I'll comment on the original issue.

aaronpuchert · 2025-09-07T20:25:48Z

clang/test/Analysis/thread-safety-lock-scope.cpp

+  Mutex local_m0;
+  Mutex local_m1;
+  LockMutexes(local_m0, local_m1);
+} // No warnings expected at end of function scope as the mutexes are function local.


Hmm, not sure I agree here. I think mutexes should be properly released even if they go out of scope.

aaronpuchert · 2025-09-07T20:28:09Z

clang/test/Analysis/thread-safety-lock-scope.cpp

+void no_local_unique_locks_held_warning() {
+  unique_lock<Mutex> ul0(m0);
+  unique_lock<Mutex> ul1(m1);
+  LockMutexes(ul0, ul1);


There are several issues here:

unique_lock isn't annotated as a mutex (lockable) or scoped lockable class. I guess we don't warn about it because it's a template, but I'd have to check.

Once you have a scoped lockable class with properly annotated destructor, this should already be handled by the destructor.

Yeah, we're still not able to emit attribute warnings on template instantiation. The reason is that we're doing Sema on ParsedAttr, but template instantiation has a regular attribute. I don't have a good idea how to extend the warning there.

aaronpuchert · 2025-09-07T20:31:59Z

clang/lib/Analysis/ThreadSafety.cpp

+        const til::SExpr *Sexp = ExitFact.sexpr();
+        const VarDecl *MutexVar = nullptr;
+
+        if (const auto *Proj = dyn_cast<til::Project>(Sexp)) {
+          if (const auto *Base = dyn_cast<til::LiteralPtr>(Proj->record()))
+            MutexVar = dyn_cast_or_null<VarDecl>(Base->clangDecl());
+        } else if (const auto *LP = dyn_cast<til::LiteralPtr>(Sexp)) {
+          MutexVar = dyn_cast_or_null<VarDecl>(LP->clangDecl());
+        }


Scoped lockables should be unlocked by the automatic destructor. I don't think we should add this code.

aaronpuchert · 2025-09-07T21:14:57Z

Commented on #156760: the issues is that we don't support scoped lockable arguments for the attributes. (That is, we support a different syntax since #110523, but we can't use that for std::lock which accepts both lockable and scoped lockable arguments, possibly even mixed.)

llvmbot added clang Clang issues not falling into any other category clang:analysis labels Sep 5, 2025

Prabhuk requested a review from PiJoules September 5, 2025 20:37

[clang][ThreadSafety] Handle mutex scope

e6af544

Before emitting warning about locks being held at the end of function scope check if the underlying mutex is function scoped or not.

Prabhuk force-pushed the lock_scope branch from ab96f50 to e6af544 Compare September 5, 2025 20:46

Prabhuk requested a review from a team as a code owner September 5, 2025 20:46

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 5, 2025

Prabhuk added 2 commits September 5, 2025 13:48

Use better variable name.

e9e92e5

Add regression tests.

a7e28b9

Prabhuk requested a review from aaronpuchert September 5, 2025 21:08

Remove libcxx test from this change.

e4e193f

PiJoules approved these changes Sep 5, 2025

View reviewed changes

Add mock unique_lock and unique_lock regression tests.

78c98c5

Extract regression test into a separate file

bd4eb50

llvmbot added the clang:static analyzer label Sep 5, 2025

Fix missing include flag

0939f0c

aaronpuchert requested changes Sep 7, 2025

View reviewed changes

Prabhuk closed this Sep 10, 2025

[clang][ThreadSafety] Handle mutex scope #157171

[clang][ThreadSafety] Handle mutex scope #157171

Uh oh!

Conversation

Prabhuk commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 5, 2025

Uh oh!

PiJoules left a comment

Choose a reason for hiding this comment

Uh oh!

Prabhuk commented Sep 5, 2025

Uh oh!

PiJoules commented Sep 5, 2025

Uh oh!

ilovepi commented Sep 5, 2025

Uh oh!

Prabhuk commented Sep 5, 2025

Uh oh!

aaronpuchert left a comment

Choose a reason for hiding this comment

Uh oh!

aaronpuchert Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpuchert Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpuchert Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpuchert Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpuchert commented Sep 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Prabhuk commented Sep 5, 2025 •

edited

Loading

llvmbot commented Sep 5, 2025 •

edited

Loading