Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Thread Safety Analysis: Warn when using negative reentrant capability #141599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clang/docs/ThreadSafetyAnalysis.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]>;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
93 changes: 62 additions & 31 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
That being said, the current implementation already handles both mechanisms:

  • UnaryOperator (!mutex): handled in the UO_LNot case
  • CXXOperatorCallExpr (mutex.operator!()): handled in the OO_Exclaim case

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.

Copy link
Member

Choose a reason for hiding this comment

The 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 ThreadSafety.cpp, but I might be overlooking something.

This is declaration-time validation, not flow analysis

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 ThreadSafety.cpp, which for example emits "cannot resolve lock expression".

} 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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand Down
18 changes: 13 additions & 5 deletions clang/test/SemaCXX/warn-thread-safety-analysis.cpp
View file
Open in desktop
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
Expand Down Expand Up @@ -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;
Expand All @@ -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}} \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Aaron pointed out, all -Wthread-safety* are DefaultIgnore. I also wouldn't want to enable this warning by default - otherwise we might also change ThreadSafetyAttributes warnings to be on by default for consistency.

What's the right thing to do here? Keep it consistent with the rest of the bunch or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is more that the RUN line for the test is not opting into the pedantic diagnostics but we're still getting the pedantic diagnostic. That's happening because we're adding ThreadSafetyPedantic to the ThreadSafety group in DiagnosticGroups.td, which means that passing -Wthread-safety will automatically enable -Wthread-safety-pedantic.

I think what we want is to leave DefaultIgnore on the diagnostic, but not add it to -Wthread-safety in DiagnosticGroups.td. So users have to explicitly pass the warning flag to enable the diagnostics.

That leaves the question of what to do if the user passes -Wthread-safety-pedantic but never passes -Wthread-safety. I suppose the result there is that they get no thread safety diagnostics, but maybe we want to catch that in the driver and tell the user "did you mean to pass -Wthread-safety as well?".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[..] passing -Wthread-safety will automatically enable -Wthread-safety-pedantic.

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.

maybe we want to catch that in the driver and tell the user "did you mean to pass -Wthread-safety as well?"

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. -Wthread-safety-attributes (which are also emitted in clang/lib/Sema/SemaDeclAttr.cpp). I think even if the user passes -Wthread-safety-pedantic alone, that's not inherently wrong, say if it's used for basic "linting" for the whole codebase where only a subset of that codebase enables full -Wthread-safety.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I'm convinced by that. Thanks!

// expected-warning{{'ThreadRole' (aka 'int') is marked reentrant but used as a negative capability; this may be contradictory}}

} // namespace Reentrancy
Loading