llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> 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. ```cpp 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 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 `````````` </details> https://github.com/llvm/llvm-project/pull/170843 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
