From 44ae49e1a72576ca6aa8835b3f72df9605516403 Mon Sep 17 00:00:00 2001 From: Aaron Puchert Date: Mon, 9 May 2022 15:34:09 +0200 Subject: [PATCH] Thread safety analysis: Handle compound assignment and ->* overloads Like regular assignment, compound assignment operators can be assumed to write to their left-hand side operand. So we strengthen the requirements there. (Previously only the default read access had been required.) Just like operator->, operator->* can also be assumed to dereference the left-hand side argument, so we require read access to the pointee. This will generate new warnings if the left-hand side has a pt_guarded_by attribute. This overload is rarely used, but it was trivial to add, so why not. (Supporting the builtin operator requires changes to the TIL.) Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D124966 --- clang/docs/ReleaseNotes.rst | 3 ++ clang/lib/Analysis/ThreadSafety.cpp | 25 +++++++++++---- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 37 ++++++++++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 73fb5a4..a82ae88 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -204,6 +204,9 @@ Improvements to Clang's diagnostics ``-Wimplicit-int``. - ``-Wmisexpect`` warns when the branch weights collected during profiling conflict with those added by ``llvm.expect``. +- ``-Wthread-safety-analysis`` now considers overloaded compound assignment and + increment/decrement operators as writing to their first argument, thus + requiring an exclusive lock if the argument is guarded. Non-comprehensive list of changes in this release ------------------------------------------------- diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index b8fe600..03bbf07 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1988,16 +1988,27 @@ void BuildLockset::VisitCallExpr(const CallExpr *Exp) { examineArguments(CE->getDirectCallee(), CE->arg_begin(), CE->arg_end()); } else if (const auto *OE = dyn_cast(Exp)) { - auto OEop = OE->getOperator(); + OverloadedOperatorKind OEop = OE->getOperator(); switch (OEop) { - case OO_Equal: { - const Expr *Target = OE->getArg(0); - const Expr *Source = OE->getArg(1); - checkAccess(Target, AK_Written); - checkAccess(Source, AK_Read); + case OO_Equal: + case OO_PlusEqual: + case OO_MinusEqual: + case OO_StarEqual: + case OO_SlashEqual: + case OO_PercentEqual: + case OO_CaretEqual: + case OO_AmpEqual: + case OO_PipeEqual: + case OO_LessLessEqual: + case OO_GreaterGreaterEqual: + checkAccess(OE->getArg(1), AK_Read); + LLVM_FALLTHROUGH; + case OO_PlusPlus: + case OO_MinusMinus: + checkAccess(OE->getArg(0), AK_Written); break; - } case OO_Star: + case OO_ArrowStar: case OO_Arrow: case OO_Subscript: if (!(OEop == OO_Star && OE->getNumArgs() > 1)) { diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 9cd44fb..ea229fe 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -82,6 +82,9 @@ private: T* ptr_; }; +template +U& operator->*(const SmartPtr& ptr, U T::*p) { return ptr->*p; } + // For testing destructor calls and cleanup. class MyString { @@ -4338,6 +4341,21 @@ public: void operator()() { } + Data& operator+=(int); + Data& operator-=(int); + Data& operator*=(int); + Data& operator/=(int); + Data& operator%=(int); + Data& operator^=(int); + Data& operator&=(int); + Data& operator|=(int); + Data& operator<<=(int); + Data& operator>>=(int); + Data& operator++(); + Data& operator++(int); + Data& operator--(); + Data& operator--(int); + private: int dat; }; @@ -4404,6 +4422,20 @@ public: // expected-warning {{reading variable 'datap1_' requires holding mutex 'mu_'}} data_ = *datap2_; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} \ // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}} + data_ += 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ -= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ *= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ /= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ %= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ ^= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ &= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ |= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ <<= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_ >>= 1; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + ++data_; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_++; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + --data_; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} + data_--; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} data_[0] = 0; // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}} (*datap2_)[0] = 0; // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}} @@ -4923,6 +4955,8 @@ class SmartPtr_PtGuardedBy_Test { SmartPtr sp GUARDED_BY(mu1) PT_GUARDED_BY(mu2); SmartPtr sq GUARDED_BY(mu1) PT_GUARDED_BY(mu2); + static constexpr int Cell::*pa = &Cell::a; + void test1() { mu1.ReaderLock(); mu2.Lock(); @@ -4931,6 +4965,7 @@ class SmartPtr_PtGuardedBy_Test { if (*sp == 0) doSomething(); *sp = 0; sq->a = 0; + sq->*pa = 0; if (sp[0] == 0) doSomething(); sp[0] = 0; @@ -4946,6 +4981,7 @@ class SmartPtr_PtGuardedBy_Test { if (*sp == 0) doSomething(); // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}} *sp = 0; // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}} sq->a = 0; // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}} + sq->*pa = 0; // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}} if (sp[0] == 0) doSomething(); // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}} sp[0] = 0; // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}} @@ -4962,6 +4998,7 @@ class SmartPtr_PtGuardedBy_Test { if (*sp == 0) doSomething(); // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}} *sp = 0; // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}} sq->a = 0; // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}} + sq->*pa = 0; // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}} if (sp[0] == 0) doSomething(); // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}} sp[0] = 0; // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}} -- 2.7.4