-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Thread Safety Analysis: Warn when using negative reentrant capability #141599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
Comment on lines
-323
to
+341
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this might not be the best place to put this. There are two different mechanisms to generate negative capabilities:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dug into if there's a sensible way to warn in ThreadSafety.cpp -- unsuccessfully -- maybe I'm missing something.
What is currently missing with the SemaDeclAttr.cpp implementation? This is declaration-time validation, not flow analysis -- we could stick it in ThreadSafetyAnalyzer::runAnalysis where it processes RequiresCapabilityAttr, but that runs on function definitions and generally feels wrong to add such checks there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll take a look myself. My intuition says that it should be easy in
That's correct, but in Sema we mainly want to check the expression from a C++ point of view, which means we want to check types. And the types are fine in this case. There is another layer in |
||
} 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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh, the reason this test passes despite the If we want the diagnostic to be ignored by default, we'd leave the group out of So I think we should drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As Aaron pointed out, all What's the right thing to do here? Keep it consistent with the rest of the bunch or something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is more that the I think what we want is to leave That leaves the question of what to do if the user passes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That was intentional initially, to discourage users from normally contradictory usage. But you're right, *-pedantic warnings probably shouldn't be on by default - that itself is contradictory. I fixed that.
I looked into that and there's no clean way to do that, or maybe I was looking in the wrong place. At the end of the day, this problem also exists for e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think I'm convinced by that. Thanks! |
||
|
||
} // namespace Reentrancy |
Uh oh!
There was an error while loading. Please reload this page.