[clang] Thread Safety Analysis: Warn when using negative reentrant capability (PR #141599)
via cfe-commits
cfe-commits at lists.llvm.org
Tue May 27 06:37:32 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Marco Elver (melver)
<details>
<summary>Changes</summary>
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, which is enabled by default, to warn about using a reentrant capability as a negative capability: this usage is likely contradictory.
Users that explicitly want this behaviour are free to compile with -Wno-thread-safety-pedantic.
---
Full diff: https://github.com/llvm/llvm-project/pull/141599.diff
4 Files Affected:
- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5)
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+62-31)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+9-1)
``````````diff
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index ff1dfc3e40d1a..4f92dd03230d5 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1259,6 +1259,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]>;
@@ -1268,6 +1269,7 @@ def ThreadSafety : DiagGroup<"thread-safety",
[ThreadSafetyAttributes,
ThreadSafetyAnalysis,
ThreadSafetyPrecise,
+ ThreadSafetyPedantic,
ThreadSafetyReference]>;
def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b63cc8a11b136..86a9f293979dc 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4222,6 +4222,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 enabled by default
+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 54bac40982eda..75d3d938cb8b3 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -255,22 +255,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) {
@@ -286,51 +291,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
@@ -419,11 +449,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;
@@ -495,7 +526,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..d8a3c9bf0b0c8 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -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
``````````
</details>
https://github.com/llvm/llvm-project/pull/141599
More information about the cfe-commits
mailing list