https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/141599
>From c66172eacbceb702370a54dfbcdae7dcb7bef4c5 Mon Sep 17 00:00:00 2001
From: Marco Elver <el...@google.com>
Date: Tue, 27 May 2025 15:12:11 +0200
Subject: [PATCH] Thread Safety Analysis: Warn when using negative reentrant
capability
The purpose of negative capabilities is documented as helping to prevent
double locking, which is not an issue for most reentrant capabilities
(such as mutexes).
Introduce a pedantic warning group to warn about using a reentrant
capability as a negative capability: this usage is likely contradictory.
---
clang/docs/ReleaseNotes.rst | 3 +
clang/docs/ThreadSafetyAnalysis.rst | 1 +
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td | 5 +
clang/lib/Sema/SemaDeclAttr.cpp | 93 ++++++++++++-------
.../SemaCXX/warn-thread-safety-analysis.cpp | 18 +++-
6 files changed, 85 insertions(+), 36 deletions(-)
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<ThreadSafetyPrecise>, 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<ThreadSafetyPedantic>, DefaultIgnore;
+
// Verbose thread safety warnings
def warn_thread_safety_verbose : Warning<"thread safety verbose warning">,
InGroup<ThreadSafetyVerbose>, 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<TypeDecl *> 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<CapabilityAttr>(RT->getDecl());
+ RecordDecl *RD = RT->getDecl();
+ if (checkRecordDeclForAttr<CapabilityAttr>(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<ScopedLockableAttr>(RT->getDecl());
}
-static bool checkTypedefTypeForCapability(QualType Ty) {
+static std::optional<TypeDecl *> checkTypedefTypeForCapability(QualType Ty) {
const auto *TD = Ty->getAs<TypedefType>();
if (!TD)
- return false;
+ return std::nullopt;
TypedefNameDecl *TN = TD->getDecl();
if (!TN)
- return false;
+ return std::nullopt;
- return TN->hasAttr<CapabilityAttr>();
-}
-
-static bool typeHasCapability(Sema &S, QualType Ty) {
- if (checkTypedefTypeForCapability(Ty))
- return true;
+ if (TN->hasAttr<CapabilityAttr>())
+ 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<TypeDecl *> 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<CastExpr>(Ex))
- return isCapabilityExpr(S, E->getSubExpr());
+ return validateCapabilityExpr(S, AL, E->getSubExpr(), Neg);
else if (const auto *E = dyn_cast<ParenExpr>(Ex))
- return isCapabilityExpr(S, E->getSubExpr());
+ return validateCapabilityExpr(S, AL, E->getSubExpr(), Neg);
else if (const auto *E = dyn_cast<UnaryOperator>(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<BinaryOperator>(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<CXXOperatorCallExpr>(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<ReentrantCapabilityAttr>()) {
+ 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<ValueDecl>(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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
