[ThreadSafety] Ignore parameter destructors

[usx95]

Fixes a false positive in thread safety analysis when function parameters have lock/unlock annotations in their constructor/destructor.

PR #169320 added destructors to the CFG, which introduced false positives in thread safety analysis for function parameters. According to expr.call#6, parameter initialization occurs in the caller's context while destruction occurs in the callee's context.

class A {
 public:
  A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); }
  ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
 private:
  Mutex mu_;
};

int do_something(A a) {  // Previously: false positive warning
  return 0;
}

Previously, thread safety analysis would see the destructor (unlock) in do_something but not the corresponding constructor (lock) that happened in the caller's context, causing a false positive.

[usx95]

• [ThreadSafety] Ignore parameter destructors #170843 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

Fixes a false positive in thread safety analysis when function parameters have lock/unlock annotations in their constructor/destructor.

PR #169320 added destructors to the CFG, which introduced false positives in thread safety analysis for function parameters. According to [expr.call#6](https://eel.is/c++draft/expr.call#6), parameter initialization occurs in the caller's context while destruction occurs in the callee's context.

class A {
 public:
  A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); }
  ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
 private:
  Mutex mu_;
};

int do_something(A a) {  // Previously: false positive warning
  return 0;
}

Previously, thread safety analysis would see the destructor (unlock) in do_something but not the corresponding constructor (lock) that happened in the caller's context, causing a false positive.

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/ThreadSafety.cpp (+4)
• (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+23)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index a25bd6007d5ed..12bcf2a78821e 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -43,6 +43,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TrailingObjects.h"
 #include "llvm/Support/raw_ostream.h"
@@ -2820,6 +2821,9 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
         case CFGElement::AutomaticObjectDtor: {
           CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
           const auto *DD = AD.getDestructorDecl(AC.getASTContext());
+          // Ignore parameter dtors: their ctors happen in caller context.
+          if (isa_and_nonnull<ParmVarDecl>(AD.getVarDecl()))
+            break;
           if (!DD || !DD->hasAttrs())
             break;
 
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 0e91639a271c5..203ea424020b6 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -7622,3 +7622,26 @@ void testLoopConditionalReassignment(Foo *f1, Foo *f2, bool cond) {
   ptr->mu.Unlock(); // expected-warning{{releasing mutex 'ptr->mu' that was not held}}
 } // expected-warning{{mutex 'f1->mu' is still held at the end of function}}
 }  // namespace CapabilityAliases
+
+namespace test_function_param_lock_unlock {
+class A {
+ public:
+  A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); }
+  ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
+ private:
+  Mutex mu_;
+};
+int do_something(A a) { return 0; }
+
+// Unlock in dtor without lock in ctor.
+// FIXME: We cannot detect that we are releasing a lock that was never held!
+class B {
+ public:
+  B() {}
+  B(int) {}
+  ~B() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
+ private:
+  Mutex mu_;
+};
+int do_something(B b) { return 0; }
+} // namespace test_function_param_lock_unlock

[melver]

In general LGTM, since I think you're only restoring previous behavior.

[usx95]

I will land this for now to unblock the existing breakages. Happy to address more comments post-commit.

[AaronBallman]

LGTM!