Skip to content

[clang][TSA] Make RequiresCapability a DeclOrType attribute #67095

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[clang][TSA] Make RequiresCapability a DeclOrType attribute #67095

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

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3902,7 +3902,7 @@ def ReleaseCapability : InheritableAttr {
let Documentation = [ReleaseCapabilityDocs];
}

def RequiresCapability : InheritableAttr {
def RequiresCapability : DeclOrTypeAttr {
let Spellings = [Clang<"requires_capability", 0>,
Clang<"exclusive_locks_required", 0>,
Clang<"requires_shared_capability", 0>,
Expand All @@ -3912,16 +3912,16 @@ def RequiresCapability : InheritableAttr {
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
let Subjects = SubjectList<[Function, ParmVar]>;
let Subjects = SubjectList<[Function, FunctionPointer, ParmVar]>;
let Accessors = [Accessor<"isShared", [Clang<"requires_shared_capability", 0>,
Clang<"shared_locks_required", 0>]>];
let Documentation = [Undocumented];
let Documentation = [ThreadSafetyDocs];
}

def NoThreadSafetyAnalysis : InheritableAttr {
let Spellings = [Clang<"no_thread_safety_analysis">];
let Subjects = SubjectList<[Function]>;
let Documentation = [Undocumented];
let Documentation = [ThreadSafetyDocs];
let SimpleHandler = 1;
}

Expand All @@ -3933,7 +3933,7 @@ def GuardedBy : InheritableAttr {
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
let Subjects = SubjectList<[Field, SharedVar]>;
let Documentation = [Undocumented];
let Documentation = [ThreadSafetyDocs];
}

def PtGuardedBy : InheritableAttr {
Expand All @@ -3944,7 +3944,7 @@ def PtGuardedBy : InheritableAttr {
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
let Subjects = SubjectList<[Field, SharedVar]>;
let Documentation = [Undocumented];
let Documentation = [ThreadSafetyDocs];
}

def AcquiredAfter : InheritableAttr {
Expand Down
7 changes: 7 additions & 0 deletions clang/include/clang/Basic/AttrDocs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9283,3 +9283,10 @@ Declares that a function potentially allocates heap memory, and prevents any pot
of ``nonallocating`` by the compiler.
}];
}

def ThreadSafetyDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
Part of Thread Safety Analysis (TSA) in Clang, as documented at https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.
}];
}
6 changes: 6 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14106,6 +14106,12 @@ class Sema final : public SemaBase {
LocalInstantiationScope &Scope,
const MultiLevelTemplateArgumentList &TemplateArgs);

public:
void checkAttrArgsAreCapabilityObjs(Decl *D, const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args,
unsigned Sidx = 0,
bool ParamIdxOk = false);

int ParsingClassDepth = 0;

class SavePendingParsedClassStateRAII {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/AST/TypePrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2098,6 +2098,9 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::ExtVectorType:
OS << "ext_vector_type";
break;
case attr::RequiresCapability:
OS << "requires_capability(...)";
break;
}
OS << "))";
}
Expand Down
43 changes: 20 additions & 23 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,12 +339,10 @@ static bool isCapabilityExpr(Sema &S, const Expr *Ex) {
/// \param Sidx The attribute argument index to start checking with.
/// \param ParamIdxOk Whether an argument can be indexing into a function
/// parameter list.
static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args,
unsigned Sidx = 0,
bool ParamIdxOk = false) {
if (Sidx == AL.getNumArgs()) {
void Sema::checkAttrArgsAreCapabilityObjs(Decl *D, const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args,
unsigned Sidx, bool ParamIdxOk) {
if (D && Sidx == AL.getNumArgs()) {
// If we don't have any capability arguments, the attribute implicitly
// refers to 'this'. So we need to make sure that 'this' exists, i.e. we're
// a non-static method, and that the class is a (scoped) capability.
Expand All @@ -354,11 +352,10 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
// FIXME -- need to check this again on template instantiation
if (!checkRecordDeclForAttr<CapabilityAttr>(RD) &&
!checkRecordDeclForAttr<ScopedLockableAttr>(RD))
S.Diag(AL.getLoc(),
diag::warn_thread_attribute_not_on_capability_member)
Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_capability_member)
<< AL << MD->getParent();
} else {
S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
<< AL;
}
}
Expand All @@ -383,7 +380,7 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,

// We allow constant strings to be used as a placeholder for expressions
// that are not valid C++ syntax, but warn that they are ignored.
S.Diag(AL.getLoc(), diag::warn_thread_attribute_ignored) << AL;
Diag(AL.getLoc(), diag::warn_thread_attribute_ignored) << AL;
Args.push_back(ArgExp);
continue;
}
Expand All @@ -402,7 +399,7 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
const RecordType *RT = getRecordType(ArgTy);

// Now check if we index into a record type function param.
if(!RT && ParamIdxOk) {
if (D && !RT && ParamIdxOk) {
const auto *FD = dyn_cast<FunctionDecl>(D);
const auto *IL = dyn_cast<IntegerLiteral>(ArgExp);
if(FD && IL) {
Expand All @@ -411,8 +408,8 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
uint64_t ParamIdxFromOne = ArgValue.getZExtValue();
uint64_t ParamIdxFromZero = ParamIdxFromOne - 1;
if (!ArgValue.isStrictlyPositive() || ParamIdxFromOne > NumParams) {
S.Diag(AL.getLoc(),
diag::err_attribute_argument_out_of_bounds_extra_info)
Diag(AL.getLoc(),
diag::err_attribute_argument_out_of_bounds_extra_info)
<< AL << Idx + 1 << NumParams;
continue;
}
Expand All @@ -424,8 +421,8 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
// expression have 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))
S.Diag(AL.getLoc(), diag::warn_thread_attribute_argument_not_lockable)
if (!typeHasCapability(*this, ArgTy) && !isCapabilityExpr(*this, ArgExp))
Diag(AL.getLoc(), diag::warn_thread_attribute_argument_not_lockable)
<< AL << ArgTy;

Args.push_back(ArgExp);
Expand Down Expand Up @@ -460,7 +457,7 @@ static bool checkGuardedByAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
Expr *&Arg) {
SmallVector<Expr *, 1> Args;
// check that all arguments are lockable objects
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size != 1)
return false;
Expand Down Expand Up @@ -502,7 +499,7 @@ static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
}

// Check that all arguments are lockable objects.
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
if (Args.empty())
return false;

Expand Down Expand Up @@ -533,7 +530,7 @@ static bool checkLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args) {
// zero or more arguments ok
// check that all arguments are lockable objects
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 0, /*ParamIdxOk=*/true);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args, 0, /*ParamIdxOk=*/true);

return true;
}
Expand Down Expand Up @@ -612,15 +609,15 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
}

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

return true;
}

static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
// check that the argument is lockable object
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size == 0)
return;
Expand All @@ -638,7 +635,7 @@ static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {

// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size == 0)
return;
Expand Down Expand Up @@ -6269,7 +6266,7 @@ static void handleReleaseCapabilityAttr(Sema &S, Decl *D,
return;
// Check that all arguments are lockable objects.
SmallVector<Expr *, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 0, true);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args, 0, true);

D->addAttr(::new (S.Context) ReleaseCapabilityAttr(S.Context, AL, Args.data(),
Args.size()));
Expand All @@ -6286,7 +6283,7 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl *D,

// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
if (Args.empty())
return;

Expand Down
25 changes: 25 additions & 0 deletions clang/lib/Sema/SemaType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8682,6 +8682,25 @@ static void HandleAnnotateTypeAttr(TypeProcessingState &State,
CurType = State.getAttributedType(AnnotateTypeAttr, CurType, CurType);
}

static void HandleRequiresCapabilityAttr(TypeProcessingState &State,
QualType &CurType,
const ParsedAttr &PA) {
Sema &S = State.getSema();

if (PA.getNumArgs() < 1) {
// Already diganosed elsewhere, just ignore.
return;
}

llvm::SmallVector<Expr *, 4> Args;
Args.reserve(PA.getNumArgs() - 1);
State.getSema().checkAttrArgsAreCapabilityObjs(/*Decl=*/nullptr, PA, Args);

auto *RCAttr =
RequiresCapabilityAttr::Create(S.Context, Args.data(), Args.size(), PA);
CurType = State.getAttributedType(RCAttr, CurType, CurType);
}

static void HandleLifetimeBoundAttr(TypeProcessingState &State,
QualType &CurType,
ParsedAttr &Attr) {
Expand Down Expand Up @@ -9034,6 +9053,12 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type,
attr.setUsedAsTypeAttr();
break;
}

case ParsedAttr::AT_RequiresCapability: {
HandleRequiresCapabilityAttr(state, type, attr);
attr.setUsedAsTypeAttr();
break;
}
Comment on lines +9057 to +9061
Copy link
Collaborator

@AaronBallman AaronBallman Nov 3, 2023

Choose a reason for hiding this comment

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

Okay, maybe we do want to do this change separately. I think I was wrong about modeling after this; I think we want it to be done like handleFunctionTypeAttr() and FUNCTION_TYPE_ATTRS_CASELIST so that it becomes part of the FunctionType itself. That's what would get us the diagnostics on type mismatches (we look through AttributedType as sugar:

llvm-project/clang/include/clang/AST/Type.h

Line 4999 in 28b7e28

bool isSugared() const { return true; }
)
Copy link
Collaborator

@AaronBallman AaronBallman Dec 6, 2023

Choose a reason for hiding this comment

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

Hmmmm, my intuition is that if we don't do this work now, we'll forever be stuck with the current behavior where we don't diagnose pointer mismatches because someone will rely on that continuing to work. So maybe we do want to do this work now...

CC @erichkeane @aaronpuchert for opinions on whether we think this needs to diagnose mismatches in the initial offering of it as a type attribute.
Copy link
Collaborator

@erichkeane erichkeane Dec 6, 2023

Choose a reason for hiding this comment

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

I think I agree Aaron, if we don't diagnose now, it'll be pretty ugly to do it later.
Copy link
Contributor Author

@tbaederr tbaederr Jan 3, 2024

Choose a reason for hiding this comment

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

Meh, it seems like adding a pointer to FunctionType::ExtInfo would blow it up a lot, and just a bit isn't enough to save the information.
Copy link
Collaborator

@erichkeane erichkeane Jan 22, 2024

Choose a reason for hiding this comment

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

There were a few changes there recently... i wonder if it is time to just accept that we're going all-in on trailing-storage for this type. We're now doing it for the ARM SVE types, plus the 'lesser used' structure, so perhaps the 'capability pointer' can end up there too. WDYT?
Copy link
Contributor Author

@tbaederr tbaederr Feb 6, 2024

Choose a reason for hiding this comment

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

@AaronBallman Does this sound like the way to go to you as well?
Copy link
Collaborator

@AaronBallman AaronBallman Feb 6, 2024

Choose a reason for hiding this comment

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

No, we don't want to modify FunctionType::ExtInfo, I think what @erichkeane was talking about is that we added trailing objects to FunctionProtoType that we can squirrel lesser-used data into. That might be reasonable here because I cannot imagine anyone using thread safety attributes with K&R C interfaces, but that would be the downside to putting the data on FunctionProtoType.
Copy link
Collaborator

@erichkeane erichkeane Feb 6, 2024

Choose a reason for hiding this comment

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

It would kind of stink to have to do it in 2 places, but it could go in trailing-storage/etc on FunctionProtoType and FunctionNoProtoType, then just disambiguate it with a function. So it doesn't need to end up in FunctionType::ExtInfo or FunctionType itself.
}

// Handle attributes that are defined in a macro. We do not want this to be
Expand Down
13 changes: 13 additions & 0 deletions clang/test/Sema/warn-thread-safety-analysis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,19 @@ int main(void) {
mutex_exclusive_unlock(late_parsing.a_mutex_defined_very_late);
mutex_exclusive_unlock(late_parsing.a_mutex_defined_late);
#endif
/// Function pointers
{
int __attribute__((requires_capability(&mu1))) (*function_ptr)(int) = Foo_fun1;

function_ptr(5); // expected-warning {{calling function 'function_ptr' requires holding mutex 'mu1'}}

mutex_exclusive_lock(&mu1);
int five = function_ptr(5);
mutex_exclusive_unlock(&mu1);

int (*function_ptr2)(int) = function_ptr;
function_ptr2(5);
}

Copy link
Collaborator

@AaronBallman AaronBallman Oct 19, 2023

Choose a reason for hiding this comment

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

We'll also need changes showing that we properly diagnose all the usual stuff from the decl form of the attribute but with the type form instead, plus additional coverage for new diagnostics like conversions between function pointer types with/out the attribute.
Copy link
Contributor Author

@tbaederr tbaederr Apr 23, 2025

Choose a reason for hiding this comment

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

There are no such diagnostics when converting between function pointers. Are there supposed to be any, without adding special handling for this? I quickly checked and noreturn has the same behavior.
return 0;
}
Expand Down
2 changes: 2 additions & 0 deletions clang/test/SemaCXX/warn-thread-safety-parsing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,8 @@ void elr_function_args() EXCLUSIVE_LOCKS_REQUIRED(mu1, mu2);

int elr_testfn(int y) EXCLUSIVE_LOCKS_REQUIRED(mu1);

int EXCLUSIVE_LOCKS_REQUIRED(mu1) (*function_ptr)(int);

int elr_testfn(int y) {
int x EXCLUSIVE_LOCKS_REQUIRED(mu1) = y; // \
// expected-warning {{'exclusive_locks_required' attribute only applies to functions}}
Expand Down
Loading