[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
Pierre d'Herbemont via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 5 07:39:56 PDT 2024
https://github.com/pdherbemont updated https://github.com/llvm/llvm-project/pull/94216
>From 94212789ca0b22380de88521279056ffdce5b3e6 Mon Sep 17 00:00:00 2001
From: Pierre d'Herbemont <pdherbemont at apple.com>
Date: Wed, 29 May 2024 11:11:03 +0200
Subject: [PATCH] Support [[guarded_by(mutex)]] and friends attributes inside C
struct
Today, it's only supported inside C++ classes or top level C/C++
declaration.
I mostly copied and adapted over the code from the
[[counted_by(value)]] lookup.
I had to change ast-dump-color.cpp because of the way the expression
marking now marks the mutex as "used" instead of "referenced". If I
change the expression parsing to use
`ExpressionEvaluationContext::Unevaluated` then I get the mutex as
"referenced" but I lose the C++ check for "invalid use of non-static
data member".
Then I enabled the use of the experimental late parsing used by
[[counted_by]] because that will enable the adoption of attributes
without restructuring the data layout as requested by @rapidsna.
---
clang/docs/ReleaseNotes.rst | 4 ++
clang/include/clang/Basic/Attr.td | 8 +--
clang/include/clang/Parse/Parser.h | 7 +++
clang/include/clang/Sema/Sema.h | 7 +--
clang/lib/Parse/ParseDecl.cpp | 62 ++++++++++++++++++-
clang/lib/Sema/SemaExpr.cpp | 4 +-
clang/test/AST/ast-dump-color.cpp | 4 +-
clang/test/Sema/warn-thread-safety-analysis.c | 17 +++++
.../SemaCXX/warn-thread-safety-parsing.cpp | 6 +-
9 files changed, 101 insertions(+), 18 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 99580c0d28a4f..c6bcf25d95dab 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -451,6 +451,10 @@ Attribute Changes in Clang
size_t count;
};
+- The ``guarded_by``, ``pt_guarded_by``, ``acquired_after``, ``acquired_before``
+ attributes now support referencing struct members in C. The arguments are also
+ now late parsed when ``-fexperimental-late-parse-attributes`` is passed like
+ for ``counted_by``.
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index c6f08860f203a..cb424965732b3 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3633,7 +3633,7 @@ def NoThreadSafetyAnalysis : InheritableAttr {
def GuardedBy : InheritableAttr {
let Spellings = [GNU<"guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = LateAttrParseStandard;
+ let LateParsed = LateAttrParseExperimentalExt;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3644,7 +3644,7 @@ def GuardedBy : InheritableAttr {
def PtGuardedBy : InheritableAttr {
let Spellings = [GNU<"pt_guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = LateAttrParseStandard;
+ let LateParsed = LateAttrParseExperimentalExt;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3655,7 +3655,7 @@ def PtGuardedBy : InheritableAttr {
def AcquiredAfter : InheritableAttr {
let Spellings = [GNU<"acquired_after">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = LateAttrParseStandard;
+ let LateParsed = LateAttrParseExperimentalExt;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3666,7 +3666,7 @@ def AcquiredAfter : InheritableAttr {
def AcquiredBefore : InheritableAttr {
let Spellings = [GNU<"acquired_before">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = LateAttrParseStandard;
+ let LateParsed = LateAttrParseExperimentalExt;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index d054b8cf0d240..fa60ef0515df1 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3128,6 +3128,13 @@ class Parser : public CodeCompletionHandler {
IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
ParsedAttr::Form Form);
+ void ParseGuardedByAttribute(IdentifierInfo &AttrName,
+ SourceLocation AttrNameLoc,
+ ParsedAttributes &Attrs,
+ IdentifierInfo *ScopeName,
+ SourceLocation ScopeLoc, SourceLocation *EndLoc,
+ ParsedAttr::Form Form);
+
void ParseTypeofSpecifier(DeclSpec &DS);
SourceLocation ParseDecltypeSpecifier(DeclSpec &DS);
void AnnotateExistingDecltypeSpecifier(const DeclSpec &DS,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7dea2b6826cfd..5207a03bb7853 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5078,7 +5078,7 @@ class Sema final : public SemaBase {
enum ExpressionKind {
EK_Decltype,
EK_TemplateArgument,
- EK_BoundsAttrArgument,
+ EK_AttrArgument,
EK_Other
} ExprContext;
@@ -5185,10 +5185,9 @@ class Sema final : public SemaBase {
return const_cast<Sema *>(this)->parentEvaluationContext();
};
- bool isBoundsAttrContext() const {
+ bool isAttrContext() const {
return ExprEvalContexts.back().ExprContext ==
- ExpressionEvaluationContextRecord::ExpressionKind::
- EK_BoundsAttrArgument;
+ ExpressionEvaluationContextRecord::ExpressionKind::EK_AttrArgument;
}
/// Increment when we find a reference; decrement when we find an ignored
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index c528917437332..87af617300cc6 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -671,6 +671,13 @@ void Parser::ParseGNUAttributeArgs(
ParseBoundsAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
Form);
return;
+ } else if (AttrKind == ParsedAttr::AT_GuardedBy ||
+ AttrKind == ParsedAttr::AT_PtGuardedBy ||
+ AttrKind == ParsedAttr::AT_AcquiredAfter ||
+ AttrKind == ParsedAttr::AT_AcquiredBefore) {
+ ParseGuardedByAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
+ EndLoc, Form);
+ return;
} else if (AttrKind == ParsedAttr::AT_CXXAssume) {
ParseCXXAssumeAttributeArg(Attrs, AttrName, AttrNameLoc, EndLoc, Form);
return;
@@ -3330,6 +3337,59 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+ IdentifierInfo &AttrName, SourceLocation AttrNameLoc,
+ ParsedAttributes &Attrs, IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
+ SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+ Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+ Parens.consumeClose();
+ return;
+ }
+
+ ArgsVector ArgExprs;
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, nullptr,
+ ExpressionKind::EK_AttrArgument);
+
+ ExprResult ArgExpr(
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (ArgExpr.isInvalid()) {
+ Parens.skipToEnd();
+ return;
+ }
+
+ ArgExprs.push_back(ArgExpr.get());
+
+ auto RParens = Tok.getLocation();
+ auto &AL =
+ *Attrs.addNew(&AttrName, SourceRange(AttrNameLoc, RParens), ScopeName,
+ ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form);
+
+ if (EndLoc)
+ *EndLoc = Tok.getLocation();
+
+ if (!Tok.is(tok::r_paren)) {
+ Diag(Tok.getLocation(), diag::err_attribute_wrong_number_arguments)
+ << AL << 1;
+ Parens.skipToEnd();
+ return;
+ }
+
+ Parens.consumeClose();
+}
+
/// Bounds attributes (e.g., counted_by):
/// AttrName '(' expression ')'
void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName,
@@ -3355,7 +3415,7 @@ void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName,
Sema::ExpressionEvaluationContextRecord::ExpressionKind;
EnterExpressionEvaluationContext EC(
Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, nullptr,
- ExpressionKind::EK_BoundsAttrArgument);
+ ExpressionKind::EK_AttrArgument);
ExprResult ArgExpr(
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index a50d5c9d6cdc9..34e39b67a11c7 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -2723,7 +2723,7 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
// appertains to a type of C struct field such that the name lookup
// within a struct finds the member name, which is not the case for other
// contexts in C.
- if (isBoundsAttrContext() && !getLangOpts().CPlusPlus && S->isClassScope()) {
+ if (isAttrContext() && !getLangOpts().CPlusPlus && S->isClassScope()) {
// See if this is reference to a field of struct.
LookupResult R(*this, NameInfo, LookupMemberName);
// LookupName handles a name lookup from within anonymous struct.
@@ -3357,7 +3357,7 @@ ExprResult Sema::BuildDeclarationNameExpr(
case Decl::Field:
case Decl::IndirectField:
case Decl::ObjCIvar:
- assert((getLangOpts().CPlusPlus || isBoundsAttrContext()) &&
+ assert((getLangOpts().CPlusPlus || isAttrContext()) &&
"building reference to field in C?");
// These can't have reference type in well-formed programs, but
diff --git a/clang/test/AST/ast-dump-color.cpp b/clang/test/AST/ast-dump-color.cpp
index 87797f6bffc5b..944b3ecb9a4f7 100644
--- a/clang/test/AST/ast-dump-color.cpp
+++ b/clang/test/AST/ast-dump-color.cpp
@@ -82,13 +82,13 @@ struct Invalid {
//CHECK: {{^}}[[Blue]]| | `-[[RESET]][[GREEN]]ParmVarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:33[[RESET]]> [[Yellow]]col:33[[RESET]] [[Green]]'const Mutex &'[[RESET]]{{$}}
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[GREEN]]CXXConstructorDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:33[[RESET]]> [[Yellow]]col:33[[RESET]] implicit constexpr[[CYAN]] Mutex[[RESET]] [[Green]]'void (Mutex &&)'[[RESET]] inline{{ .*$}}
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[GREEN]]ParmVarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:33[[RESET]]> [[Yellow]]col:33[[RESET]] [[Green]]'Mutex &&'[[RESET]]{{$}}
-//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]VarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:1[[RESET]], [[Yellow]]line:25:3[[RESET]]> [[Yellow]]col:3[[RESET]] referenced[[CYAN]] mu1[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]]
+//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]VarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:1[[RESET]], [[Yellow]]line:25:3[[RESET]]> [[Yellow]]col:3[[RESET]] used[[CYAN]] mu1[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]]
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[MAGENTA]]CXXConstructExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:3[[RESET]]> [[Green]]'class Mutex':'Mutex'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]] [[Green]]'void () noexcept'[[RESET]]{{$}}
//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]VarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:18:1[[RESET]], [[Yellow]]line:25:8[[RESET]]> [[Yellow]]col:8[[RESET]][[CYAN]] mu2[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]]
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[MAGENTA]]CXXConstructExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:8[[RESET]]> [[Green]]'class Mutex':'Mutex'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]] [[Green]]'void () noexcept'[[RESET]]{{$}}
//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]VarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:26:1[[RESET]], [[Yellow]]col:5[[RESET]]> [[Yellow]]col:5[[RESET]][[CYAN]] TestExpr[[RESET]] [[Green]]'int'[[RESET]]
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[BLUE]]GuardedByAttr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:29[[RESET]], [[Yellow]]col:43[[RESET]]>{{$}}
-//CHECK: {{^}}[[Blue]]| `-[[RESET]][[MAGENTA]]DeclRefExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:40[[RESET]]> [[Green]]'class Mutex':'Mutex'[[RESET]][[Cyan]] lvalue[[RESET]][[Cyan]][[RESET]] [[GREEN]]Var[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]][[CYAN]] 'mu1'[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]] non_odr_use_unevaluated{{$}}
+//CHECK: {{^}}[[Blue]]| `-[[RESET]][[MAGENTA]]DeclRefExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:40[[RESET]]> [[Green]]'class Mutex':'Mutex'[[RESET]][[Cyan]] lvalue[[RESET]][[Cyan]][[RESET]] [[GREEN]]Var[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]][[CYAN]] 'mu1'[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]]{{$}}
//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]CXXRecordDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:28:1[[RESET]], [[Yellow]]line:30:1[[RESET]]> [[Yellow]]line:28:8[[RESET]] struct[[CYAN]] Invalid[[RESET]] definition
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXRecordDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:1[[RESET]], [[Yellow]]col:8[[RESET]]> [[Yellow]]col:8[[RESET]] implicit referenced struct[[CYAN]] Invalid[[RESET]]
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXConstructorDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:29:3[[RESET]], [[Yellow]]col:42[[RESET]]> [[Yellow]]col:29[[RESET]] invalid[[CYAN]] Invalid[[RESET]] [[Green]]'void (int)'[[RESET]]
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 642ea88ec3c96..bc01a0bb301ee 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -29,6 +29,12 @@ struct LOCKABLE Mutex {};
struct Foo {
struct Mutex *mu_;
+ struct Bar {
+ struct Mutex *other_mu ACQUIRED_AFTER(mu_);
+ struct Mutex *third_mu ACQUIRED_BEFORE(other_mu);
+ } bar;
+ int a_value GUARDED_BY(mu_);
+ int* a_ptr PT_GUARDED_BY(bar.other_mu);
};
// Declare mutex lock/unlock functions.
@@ -136,6 +142,17 @@ int main(void) {
// Cleanup happens automatically -> no warning.
}
+ foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}}
+ *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}}
+
+
+ mutex_exclusive_lock(foo_.bar.other_mu);
+ mutex_exclusive_lock(foo_.bar.third_mu); // expected-warning{{mutex 'third_mu' must be acquired before 'other_mu'}}
+ mutex_exclusive_lock(foo_.mu_); // expected-warning{{mutex 'mu_' must be acquired before 'other_mu'}}
+ mutex_exclusive_unlock(foo_.mu_);
+ mutex_exclusive_unlock(foo_.bar.other_mu);
+ mutex_exclusive_unlock(foo_.bar.third_mu);
+
return 0;
}
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 0c5b0cc85897b..1626e8375892a 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -1516,11 +1516,7 @@ class Foo {
mutable Mutex mu;
int a GUARDED_BY(mu);
- static int si GUARDED_BY(mu);
-//FIXME: Bug 32066 - Error should be emitted irrespective of C++ dialect
-#if __cplusplus <= 199711L
- // expected-error at -3 {{invalid use of non-static data member 'mu'}}
-#endif
+ static int si GUARDED_BY(mu); // expected-error {{invalid use of non-static data member 'mu'}}
static void foo() EXCLUSIVE_LOCKS_REQUIRED(mu);
//FIXME: Bug 32066 - Error should be emitted irrespective of C++ dialect
More information about the cfe-commits
mailing list