Thread Safety Analysis: Warn when using negative reentrant capability

[melver]

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.

Context: #137133 (comment)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Marco Elver (melver)

Changes

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 --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

[AaronBallman]

Thanks for the new diagnostic! I think you should also add a release note to clang/docs/ReleaseNotes.rst so users know about the new diagnostic group.

[AaronBallman]

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.

The only attribute in the group is marked DefaultIgnore, did you mean to drop that?

[aaronpuchert]

All thread-safety warnings are off-by-default, so this should be consistent. But I'm not sure we need a separate flag at all for now. (Reentrant capabilities have just been introduced, so we shouldn't break much.)

[AaronBallman]

Oooh, the reason this test passes despite the DefaultIgnore is because the diagnostic is enabled by -Wthread-safety which is the only way to enable any thread safety diagnostics.

If we want the diagnostic to be ignored by default, we'd leave the group out of -Wthread-safety but that could get awkward (what if you enable just the pedantic warning and nothing else? ew.)

So I think we should drop the DefaultIgnore above to avoid confusion.

[aaronpuchert]

On a related note, do we emit -Wthread-safety-negative for reentrant locks? I don't remember that we carved out an exception for that, and we probably should.

[aaronpuchert]

All thread-safety warnings are off-by-default, so this should be consistent. But I'm not sure we need a separate flag at all for now. (Reentrant capabilities have just been introduced, so we shouldn't break much.)

[aaronpuchert]

Hmm, this might not be the best place to put this. There are two different mechanisms to generate negative capabilities:

• the supported, but untypical case of a UnaryOperator, which basically requires the capability to be an integer,
• the more common CXXOperatorCallExpr. (Well, at least in C++. Not sure how negating a mutex would work in C.)

This code here just checks the type (that's what Sema usually does), and I think what we want to check here is better done in ThreadSafety.cpp. Once we have a CapabilityExpr this should be much easier.