Skip to content

[clang][ThreadSafety] Check trylock function success and return types #95290

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

Merged
merged 1 commit into from
Jun 24, 2024
Merged

[clang][ThreadSafety] Check trylock function success and return types #95290

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
10 changes: 10 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ C++ Specific Potentially Breaking Changes

To fix this, update libstdc++ to version 14.1.1 or greater.

- Clang now emits errors when Thread Safety Analysis trylock attributes are
applied to functions or methods with incompatible return values, such as
constructors, destructors, and void-returning functions. This only affects the
``TRY_ACQUIRE`` and ``TRY_ACQUIRE_SHARED`` attributes (and any synonyms).

ABI Changes in This Version
---------------------------
- Fixed Microsoft name mangling of implicitly defined variables used for thread
Expand Down Expand Up @@ -720,6 +725,11 @@ Bug Fixes in This Version

- Fixed `static_cast` to array of unknown bound. Fixes (#GH62863).

- Clang's Thread Safety Analysis now evaluates trylock success arguments of enum
types rather than silently defaulting to false. This fixes a class of false
negatives where the analysis failed to detect unchecked access to guarded
data.

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
52 changes: 48 additions & 4 deletions clang/docs/ThreadSafetyAnalysis.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,10 +420,17 @@ TRY_ACQUIRE(<bool>, ...), TRY_ACQUIRE_SHARED(<bool>, ...)
*Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION``

These are attributes on a function or method that tries to acquire the given
capability, and returns a boolean value indicating success or failure.
The first argument must be ``true`` or ``false``, to specify which return value
indicates success, and the remaining arguments are interpreted in the same way
as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses.
capability, and returns a boolean, integer, or pointer value indicating success
or failure.

The attribute's first argument defines whether a zero or non-zero return value
indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool``
and ``int`` literals, as well as enumerator values. *The analysis only cares
whether this success value is zero or non-zero.* This leads to some subtle
consequences, discussed in the next section.

The remaining arguments are interpreted in the same way as ``ACQUIRE``. See
:ref:`mutexheader`, below, for example uses.

Because the analysis doesn't support conditional locking, a capability is
treated as acquired after the first branch on the return value of a try-acquire
Expand All @@ -445,6 +452,43 @@ function.
}
}

Subtle Consequences of Non-Boolean Success Values
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The trylock attributes accept non-boolean expressions for the success value, but
the analysis only cares whether the value is zero or non-zero.

Suppose you define an enum with two non-zero enumerators: ``LockAcquired = 1``
and ``LockNotAcquired = 2``. If your trylock function returns ``LockAcquired``
on success and ``LockNotAcquired`` on failure, the analysis may fail to detect
access to guarded data without holding the mutex because they are both non-zero.

.. code-block:: c++
// *** Beware: this code demonstrates incorrect usage. ***

enum TrylockResult { LockAcquired = 1, LockNotAcquired = 2 };

class CAPABILITY("mutex") Mutex {
public:
TrylockResult TryLock() TRY_ACQUIRE(LockAcquired);
};

Mutex mu;
int a GUARDED_BY(mu);

void foo() {
if (mu.TryLock()) { // This branch satisfies the analysis, but permits
// unguarded access to `a`!
a = 0;
mu.Unlock();
}
}

It's also possible to return a pointer from the trylock function. Similarly, all
that matters is whether the success value is zero or non-zero. For instance, a
success value of `true` means the function returns a non-null pointer on
success.


ASSERT_CAPABILITY(...) and ASSERT_SHARED_CAPABILITY(...)
--------------------------------------------------------
Expand Down
23 changes: 18 additions & 5 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3272,11 +3272,23 @@ def warn_aligned_attr_underaligned : Warning<err_alignas_underaligned.Summary>,
def err_attribute_sizeless_type : Error<
"%0 attribute cannot be applied to sizeless type %1">;
def err_attribute_argument_n_type : Error<
"%0 attribute requires parameter %1 to be %select{int or bool|an integer "
"constant|a string|an identifier|a constant expression|a builtin function}2">;
"%0 attribute requires parameter %1 to be %select{"
"int or bool"
"|an integer constant"
"|a string"
"|an identifier"
"|a constant expression"
"|a builtin function"
"|nullptr or a bool, int, or enum literal}2">;
def err_attribute_argument_type : Error<
"%0 attribute requires %select{int or bool|an integer "
"constant|a string|an identifier}1">;
"%0 attribute requires %select{"
"int or bool"
"|an integer constant"
"|a string"
"|an identifier"
"|a constant expression"
"|a builtin function"
"|a pointer or a bool, int, or enum literal}1">;
def err_attribute_argument_out_of_range : Error<
"%0 attribute requires integer constant between %1 and %2 inclusive">;
def err_init_priority_object_attr : Error<
Expand Down Expand Up @@ -3728,7 +3740,8 @@ def warn_attribute_wrong_decl_type : Warning<
"|types and namespaces"
"|variables, functions and classes"
"|kernel functions"
"|non-K&R-style functions}2">,
"|non-K&R-style functions"
"|functions that return bool, integer, or a pointer type}2">,
InGroup<IgnoredAttributes>;
def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Summary>;
def warn_type_attribute_wrong_type : Warning<
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/ParsedAttr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,7 @@ enum AttributeArgumentNType {
AANT_ArgumentIdentifier,
AANT_ArgumentConstantExpr,
AANT_ArgumentBuiltinFunction,
AANT_ArgumentNullptrOrBoolIntOrEnumLiteral,
};

/// These constants match the enumerated choices of
Expand All @@ -1101,6 +1102,7 @@ enum AttributeDeclKind {
ExpectedFunctionVariableOrClass,
ExpectedKernelFunction,
ExpectedFunctionWithProtoType,
ExpectedFunctionReturningPointerBoolIntOrEnum,
};

inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
Expand Down
82 changes: 46 additions & 36 deletions clang/lib/Analysis/ThreadSafety.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "clang/Basic/OperatorKinds.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/Specifiers.h"
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/ImmutableMap.h"
Expand Down Expand Up @@ -1034,9 +1035,10 @@ class ThreadSafetyAnalyzer {

template <class AttrType>
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
const NamedDecl *D,
const CFGBlock *PredBlock, const CFGBlock *CurrBlock,
Expr *BrE, bool Neg);
const NamedDecl *D, const CFGBlock *PredBlock,
const CFGBlock *CurrBlock,
const ASTContext &TrylockAttrContext,
Expr *TrylockAttrSuccessValue, bool Neg);

const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C,
bool &Negate);
Expand Down Expand Up @@ -1359,17 +1361,18 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
const Expr *Exp, const NamedDecl *D,
const CFGBlock *PredBlock,
const CFGBlock *CurrBlock,
Expr *BrE, bool Neg) {
// Find out which branch has the lock
bool branch = false;
if (const auto *BLE = dyn_cast_or_null<CXXBoolLiteralExpr>(BrE))
branch = BLE->getValue();
else if (const auto *ILE = dyn_cast_or_null<IntegerLiteral>(BrE))
branch = ILE->getValue().getBoolValue();
Comment on lines -1363 to -1368
Copy link
Member

Choose a reason for hiding this comment

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

I kind of prefer the old code here. Regardless of what we accept as return type, we only care about the return type converted to bool, so other values don't really make sense as of now. (We need the IntegerLiteral for C.)

What we should document is that this isn't an equality comparison, but a comparison after conversion to bool. (Or whatever the equivalent for C is. Basically what is allowed in an if condition.)

Should we support integer/enumeration values properly, that would of course change the picture. In that case we can allow enumerators in the attribute, and the return type must be explicitly (?) convertible to type of the attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @aaronpuchert! I've reverted the change and I'll work through these comments for a reland.

My instinct is to explicitly disallow enumerator success expressions. Currently, the analysis can produce both false negatives and false positives when enumerator success expressions are used: https://godbolt.org/z/MPYdK6hYb. To my knowledge, enumerators were never actually supported in success expressions, so in some sense this will only break code that was already broken. However, I do want to be sensitive to downstream breakage based on my experience with this PR!

One suggestion was to add a flag that disables any new errors, the goal being to enabling large code bases to incrementally adapt to the new behavior. I'm just not sure how this flag would actually work.

  • If we suppress any new type errors, but otherwise apply the attribute, the analysis is now off the rails, operating on unexpected inputs.
  • If we simply ignore any attributes that would cause an error to be emitted, now the analysis can produce different kinds of incorrect results, e.g. it might complain that you're unlocking a lock that was never acquired.
  • I suppose we could mimic the current/incorrect behavior when the flag is present. Is it normally expected to keep incorrect behavior around for compatibility?

@asmok-g, do you have any thoughts? How are breaking changes such as this typically handled?

Copy link
Member

Choose a reason for hiding this comment

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

My instinct is to explicitly disallow enumerator success expressions. [...] To my knowledge, enumerators were never actually supported in success expressions, so in some sense this will only break code that was already broken.

Agreed.

Is it normally expected to keep incorrect behavior around for compatibility?

It depends. If the incorrect behavior is being relied upon, or the correction could produce hard-to-find issues like different behavior at runtime, then it could make sense to keep it. But here I find it unlikely that someone relies on this, as it does not properly work, and the additional error would at most make compilation fail and would be trivial to fix.

How are breaking changes such as this typically handled?

I'd argue that the change isn't breaking anything, it's merely diagnosing a case that was already not appropriately handled.


int branchnum = branch ? 0 : 1;
if (Neg)
branchnum = !branchnum;
const ASTContext &TrylockAttrContext,
Expr *TrylockAttrSuccess,
bool Neg) {
// Evaluate the trylock's success value as a boolean.
bool trylockSuccessValue = false;
if (!TrylockAttrSuccess->EvaluateAsBooleanCondition(
trylockSuccessValue, TrylockAttrContext,
/*InConstantContext=*/true)) {
llvm_unreachable("Trylock success value could not be evaluated.");
}

const int branchnum = Neg ? !!trylockSuccessValue : !trylockSuccessValue;

// If we've taken the trylock branch, then add the lock
int i = 0;
Expand All @@ -1390,8 +1393,15 @@ static bool getStaticBooleanValue(Expr *E, bool &TCond) {
} else if (const auto *ILE = dyn_cast<IntegerLiteral>(E)) {
TCond = ILE->getValue().getBoolValue();
return true;
} else if (auto *CE = dyn_cast<ImplicitCastExpr>(E))
} else if (auto *CE = dyn_cast<ImplicitCastExpr>(E)) {
return getStaticBooleanValue(CE->getSubExpr(), TCond);
} else if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
if (auto *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
llvm::APSInt IV = ECD->getInitVal();
TCond = IV.getBoolValue();
return true;
}
}
return false;
}

Expand Down Expand Up @@ -1497,27 +1507,27 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
// If the condition is a call to a Trylock function, then grab the attributes
for (const auto *Attr : FunDecl->attrs()) {
switch (Attr->getKind()) {
case attr::TryAcquireCapability: {
auto *A = cast<TryAcquireCapabilityAttr>(Attr);
getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
Exp, FunDecl, PredBlock, CurrBlock, A->getSuccessValue(),
Negate);
break;
};
case attr::ExclusiveTrylockFunction: {
const auto *A = cast<ExclusiveTrylockFunctionAttr>(Attr);
getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock,
A->getSuccessValue(), Negate);
break;
}
case attr::SharedTrylockFunction: {
const auto *A = cast<SharedTrylockFunctionAttr>(Attr);
getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock,
A->getSuccessValue(), Negate);
break;
}
default:
break;
case attr::TryAcquireCapability: {
auto *A = cast<TryAcquireCapabilityAttr>(Attr);
getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
Exp, FunDecl, PredBlock, CurrBlock, FunDecl->getASTContext(),
A->getSuccessValue(), Negate);
break;
};
case attr::ExclusiveTrylockFunction: {
const auto *A = cast<ExclusiveTrylockFunctionAttr>(Attr);
getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock,
FunDecl->getASTContext(), A->getSuccessValue(), Negate);
break;
}
case attr::SharedTrylockFunction: {
const auto *A = cast<SharedTrylockFunctionAttr>(Attr);
getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock,
FunDecl->getASTContext(), A->getSuccessValue(), Negate);
break;
}
default:
break;
}
}

Expand Down
34 changes: 24 additions & 10 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTMutationListener.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
Expand All @@ -25,6 +26,7 @@
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/Cuda.h"
#include "clang/Basic/DarwinSDKInfo.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/HLSLRuntime.h"
#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/LangOptions.h"
Expand Down Expand Up @@ -65,10 +67,13 @@
#include "llvm/Demangle/Demangle.h"
#include "llvm/IR/Assumptions.h"
#include "llvm/MC/MCSectionMachO.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Triple.h"
#include <cassert>
#include <optional>

using namespace clang;
Expand Down Expand Up @@ -166,13 +171,6 @@ bool Sema::checkStringLiteralArgumentAttr(const ParsedAttr &AL, unsigned ArgNum,
return checkStringLiteralArgumentAttr(AL, ArgExpr, Str, ArgLocation);
}

/// Check if the passed-in expression is of type int or bool.
static bool isIntOrBool(Expr *Exp) {
QualType QT = Exp->getType();
return QT->isBooleanType() || QT->isIntegerType();
}


// Check to see if the type is a smart pointer of some kind. We assume
// it's a smart pointer if it defines both operator-> and operator*.
static bool threadSafetyCheckIsSmartPointer(Sema &S, const RecordType* RT) {
Expand Down Expand Up @@ -608,15 +606,31 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
if (!AL.checkAtLeastNumArgs(S, 1))
return false;

if (!isIntOrBool(AL.getArgAsExpr(0))) {
// The attribute's first argument defines the success value.
const Expr *SuccessArg = AL.getArgAsExpr(0);
if (!isa<CXXNullPtrLiteralExpr>(SuccessArg) &&
Copy link
Member

Choose a reason for hiding this comment

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

I can see the appeal of nullptr in the attribute, but it suggests you can put arbitrary pointers in there, and I don't think we want that. But I can live with it.

Though it seems strange that one would return a pointer, and nullptr is success. Maybe an error message? More typical is probably to have nullptr if the lock couldn't be acquired.

In any event, since we want to able to express both the null and non-null case, conversion to bool seems the better conceptual framework. How much we need bother users with that framework is another question, but it's probably not a bad idea to bring home the idea that we're not trying to track pointers.

!isa<GNUNullExpr>(SuccessArg) && !isa<CXXBoolLiteralExpr>(SuccessArg) &&
!isa<IntegerLiteral>(SuccessArg) && !SuccessArg->getEnumConstantDecl()) {
S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
<< AL << 1 << AANT_ArgumentIntOrBool;
<< AL << 1 << AANT_ArgumentNullptrOrBoolIntOrEnumLiteral;
return false;
}

// check that all arguments are lockable objects
// All remaining arguments must be lockable objects.
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1);

// The function must return a pointer, boolean, integer, or enum. We already
// know that `D` is a function because `ExclusiveTrylockFunction` and friends
// are defined in Attr.td with subject lists that only include functions.
QualType ReturnType = D->getAsFunction()->getReturnType();
if (!ReturnType->isPointerType() && !ReturnType->isBooleanType() &&
!ReturnType->isIntegerType() && !ReturnType->isEnumeralType()) {
S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type)
<< AL << AL.isRegularKeywordAttribute()
<< ExpectedFunctionReturningPointerBoolIntOrEnum;
return false;
}

return true;
}

Expand Down
12 changes: 6 additions & 6 deletions clang/test/Sema/attr-capabilities.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ void Func18(void) __attribute__((release_capability())) {} // expected-warning {
void Func19(void) __attribute__((release_shared_capability())) {} // expected-warning {{'release_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
void Func20(void) __attribute__((release_generic_capability())) {} // expected-warning {{'release_generic_capability' attribute without capability arguments can only be applied to non-static methods of a class}}

void Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
void Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
int Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
int Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}

void Func23(void) __attribute__((try_acquire_capability(1, GUI))) {}
void Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {}
int Func23(void) __attribute__((try_acquire_capability(1, GUI))) {}
int Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {}

void Func25(void) __attribute__((try_acquire_capability())) {} // expected-error {{'try_acquire_capability' attribute takes at least 1 argument}}
void Func26(void) __attribute__((try_acquire_shared_capability())) {} // expected-error {{'try_acquire_shared_capability' attribute takes at least 1 argument}}
int Func25(void) __attribute__((try_acquire_capability())) {} // expected-error {{'try_acquire_capability' attribute takes at least 1 argument}}
int Func26(void) __attribute__((try_acquire_shared_capability())) {} // expected-error {{'try_acquire_shared_capability' attribute takes at least 1 argument}}

// Test that boolean logic works with capability attributes
void Func27(void) __attribute__((requires_capability(!GUI)));
Expand Down
Loading
Loading