diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index f13d99845ee9a..6ab7fa9664db0 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -494,6 +494,9 @@ Improvements to Clang's diagnostics :doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The feature will be default-enabled with ``-Wthread-safety`` in a future release. - The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities. +- New warning group ``-Wthread-safety-pedantic`` warns about contradictory + :doc:`ThreadSafetyAnalysis` usage patterns; currently warns about use of a + reentrant capability as a negative capability. - Clang will now do a better job producing common nested names, when producing common types for ternary operator, template argument deduction and multiple return auto deduction. - The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) and minus(-) as signed integers diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index 4fc7ff28e9931..2e62c365ce588 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -536,6 +536,7 @@ Warning flags * ``-Wthread-safety-pointer``: Checks when passing or returning pointers to guarded variables, or pointers to guarded data, as function argument or return value respectively. +* ``-Wthread-safety-pedantic``: Contradictory usage patterns. :ref:`negative` are an experimental feature, which are enabled with: diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 36fa3227fd6a6..c54281ff9dd0f 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1272,6 +1272,7 @@ def Most : DiagGroup<"most", [ def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">; def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; +def ThreadSafetyPedantic : DiagGroup<"thread-safety-pedantic">; def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">; def ThreadSafetyReference : DiagGroup<"thread-safety-reference", [ThreadSafetyReferenceReturn]>; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 451619709c087..f3fbb33c45b5d 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4254,6 +4254,11 @@ def warn_fun_requires_lock_precise : InGroup, DefaultIgnore; def note_found_mutex_near_match : Note<"found near match '%0'">; +// Pedantic thread safety warnings +def warn_thread_reentrant_with_negative_capability : Warning< + "%0 is marked reentrant but used as a negative capability; this may be contradictory">, + InGroup, DefaultIgnore; + // Verbose thread safety warnings def warn_thread_safety_verbose : Warning<"thread safety verbose warning">, InGroup, DefaultIgnore; diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index c5d5d03cc99c7..36b088e793618 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -256,22 +256,27 @@ static bool checkRecordDeclForAttr(const RecordDecl *RD) { return false; } -static bool checkRecordTypeForCapability(Sema &S, QualType Ty) { +static std::optional checkRecordTypeForCapability(Sema &S, + QualType Ty) { const RecordType *RT = getRecordType(Ty); if (!RT) - return false; + return std::nullopt; // Don't check for the capability if the class hasn't been defined yet. if (RT->isIncompleteType()) - return true; + return {nullptr}; // Allow smart pointers to be used as capability objects. // FIXME -- Check the type that the smart pointer points to. if (threadSafetyCheckIsSmartPointer(S, RT)) - return true; + return {nullptr}; - return checkRecordDeclForAttr(RT->getDecl()); + RecordDecl *RD = RT->getDecl(); + if (checkRecordDeclForAttr(RD)) + return {RD}; + + return std::nullopt; } static bool checkRecordTypeForScopedCapability(Sema &S, QualType Ty) { @@ -287,51 +292,76 @@ static bool checkRecordTypeForScopedCapability(Sema &S, QualType Ty) { return checkRecordDeclForAttr(RT->getDecl()); } -static bool checkTypedefTypeForCapability(QualType Ty) { +static std::optional checkTypedefTypeForCapability(QualType Ty) { const auto *TD = Ty->getAs(); if (!TD) - return false; + return std::nullopt; TypedefNameDecl *TN = TD->getDecl(); if (!TN) - return false; + return std::nullopt; - return TN->hasAttr(); -} - -static bool typeHasCapability(Sema &S, QualType Ty) { - if (checkTypedefTypeForCapability(Ty)) - return true; + if (TN->hasAttr()) + return {TN}; - if (checkRecordTypeForCapability(S, Ty)) - return true; + return std::nullopt; +} - return false; +/// Returns capability TypeDecl if defined, nullptr if not yet defined (maybe +/// capability), and nullopt if it definitely is not a capability. +static std::optional checkTypeForCapability(Sema &S, QualType Ty) { + if (auto TD = checkTypedefTypeForCapability(Ty)) + return TD; + if (auto TD = checkRecordTypeForCapability(S, Ty)) + return TD; + return std::nullopt; } -static bool isCapabilityExpr(Sema &S, const Expr *Ex) { +static bool validateCapabilityExpr(Sema &S, const ParsedAttr &AL, + const Expr *Ex, bool Neg = false) { // Capability expressions are simple expressions involving the boolean logic // operators &&, || or !, a simple DeclRefExpr, CastExpr or a ParenExpr. Once // a DeclRefExpr is found, its type should be checked to determine whether it // is a capability or not. if (const auto *E = dyn_cast(Ex)) - return isCapabilityExpr(S, E->getSubExpr()); + return validateCapabilityExpr(S, AL, E->getSubExpr(), Neg); else if (const auto *E = dyn_cast(Ex)) - return isCapabilityExpr(S, E->getSubExpr()); + return validateCapabilityExpr(S, AL, E->getSubExpr(), Neg); else if (const auto *E = dyn_cast(Ex)) { - if (E->getOpcode() == UO_LNot || E->getOpcode() == UO_AddrOf || - E->getOpcode() == UO_Deref) - return isCapabilityExpr(S, E->getSubExpr()); - return false; + switch (E->getOpcode()) { + case UO_LNot: + Neg = !Neg; + [[fallthrough]]; + case UO_AddrOf: + case UO_Deref: + return validateCapabilityExpr(S, AL, E->getSubExpr(), Neg); + default: + return false; + } } else if (const auto *E = dyn_cast(Ex)) { if (E->getOpcode() == BO_LAnd || E->getOpcode() == BO_LOr) - return isCapabilityExpr(S, E->getLHS()) && - isCapabilityExpr(S, E->getRHS()); + return validateCapabilityExpr(S, AL, E->getLHS(), Neg) && + validateCapabilityExpr(S, AL, E->getRHS(), Neg); return false; + } else if (const auto *E = dyn_cast(Ex)) { + if (E->getOperator() == OO_Exclaim && E->getNumArgs() == 1) { + // operator!(this) - return type is the expression to check below. + Neg = !Neg; + } } - return typeHasCapability(S, Ex->getType()); + // Base case: check the inner type for capability. + QualType Ty = Ex->getType(); + if (auto TD = checkTypeForCapability(S, Ty)) { + if (Neg && *TD != nullptr && (*TD)->hasAttr()) { + S.Diag(AL.getLoc(), diag::warn_thread_reentrant_with_negative_capability) + << Ty.getUnqualifiedType(); + } + return true; + } + + return false; } /// Checks that all attribute arguments, starting from Sidx, resolve to @@ -420,11 +450,12 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D, } } - // If the type does not have a capability, see if the components of the - // expression have capabilities. This allows for writing C code where the + // If ArgTy is not a capability, this also checks if components of the + // expression are capabilities. This allows for writing C code where the // capability may be on the type, and the expression is a capability // boolean logic expression. Eg) requires_capability(A || B && !C) - if (!typeHasCapability(S, ArgTy) && !isCapabilityExpr(S, ArgExp)) + if (!validateCapabilityExpr(S, AL, ArgExp) && + !checkTypeForCapability(S, ArgTy)) S.Diag(AL.getLoc(), diag::warn_thread_attribute_argument_not_lockable) << AL << ArgTy; @@ -496,7 +527,7 @@ static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL, // Check that this attribute only applies to lockable types. QualType QT = cast(D)->getType(); - if (!QT->isDependentType() && !typeHasCapability(S, QT)) { + if (!QT->isDependentType() && !checkTypeForCapability(S, QT)) { S.Diag(AL.getLoc(), diag::warn_thread_attribute_decl_not_lockable) << AL; return false; } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index d64ed1e5f260a..430ec1072aa3c 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-pedantic -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-pedantic -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-pedantic -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-pedantic -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s @@ -7209,12 +7209,14 @@ void testReentrantTypedef() { bit_unlock(bl); } +// Negative + reentrant capability tests. class TestNegativeWithReentrantMutex { ReentrantMutex rmu; int a GUARDED_BY(rmu); public: - void baz() EXCLUSIVE_LOCKS_REQUIRED(!rmu) { + void baz() EXCLUSIVE_LOCKS_REQUIRED(!rmu) { // \ + // expected-warning{{'ReentrantMutex' is marked reentrant but used as a negative capability; this may be contradictory}} rmu.Lock(); rmu.Lock(); a = 0; @@ -7223,4 +7225,10 @@ class TestNegativeWithReentrantMutex { } }; +typedef int __attribute__((capability("role"), reentrant_capability)) ThreadRole; +ThreadRole FlightControl1, FlightControl2; +void dispatch_log(const char *msg) __attribute__((requires_capability(!FlightControl1 && !FlightControl2))) {} // \ + // expected-warning{{'ThreadRole' (aka 'int') is marked reentrant but used as a negative capability; this may be contradictory}} \ + // expected-warning{{'ThreadRole' (aka 'int') is marked reentrant but used as a negative capability; this may be contradictory}} + } // namespace Reentrancy