https://github.com/pdherbemont updated https://github.com/llvm/llvm-project/pull/94216
>From 313335523ecdfec8cbf7e703cb720bb4adc81a52 Mon Sep 17 00:00:00 2001 From: Pierre d'Herbemont <pdherbem...@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/docs/ThreadSafetyAnalysis.rst | 6 - clang/include/clang/Basic/Attr.td | 8 +- clang/include/clang/Parse/Parser.h | 14 +++ clang/include/clang/Sema/Sema.h | 7 +- clang/lib/Parse/ParseDecl.cpp | 117 +++++++++++++++++- clang/lib/Sema/SemaExpr.cpp | 4 +- clang/test/AST/ast-dump-color.cpp | 4 +- clang/test/Sema/warn-thread-safety-analysis.c | 20 ++- .../SemaCXX/warn-thread-safety-parsing.cpp | 6 +- 10 files changed, 165 insertions(+), 25 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/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index dcde0c706c704..6eefa306e3c89 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -764,12 +764,6 @@ doesn't know that munl.mu == mutex. The SCOPED_CAPABILITY attribute handles aliasing for MutexLocker, but does so only for that particular pattern. -ACQUIRED_BEFORE(...) and ACQUIRED_AFTER(...) are currently unimplemented. -------------------------------------------------------------------------- - -To be fixed in a future update. - - .. _mutexheader: mutex.h 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..9e11078382756 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -3128,6 +3128,20 @@ 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 ParseAcquiredAttribute(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..e8c4ed10910d5 100644 --- a/clang/lib/Parse/ParseDecl.cpp +++ b/clang/lib/Parse/ParseDecl.cpp @@ -671,6 +671,16 @@ void Parser::ParseGNUAttributeArgs( ParseBoundsAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc, Form); return; + } else if (AttrKind == ParsedAttr::AT_GuardedBy || + AttrKind == ParsedAttr::AT_PtGuardedBy) { + ParseGuardedByAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc, + EndLoc, Form); + return; + } else if (AttrKind == ParsedAttr::AT_AcquiredAfter || + AttrKind == ParsedAttr::AT_AcquiredBefore) { + ParseAcquiredAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc, + EndLoc, Form); + return; } else if (AttrKind == ParsedAttr::AT_CXXAssume) { ParseCXXAssumeAttributeArg(Attrs, AttrName, AttrNameLoc, EndLoc, Form); return; @@ -3330,6 +3340,111 @@ 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 = RParens; + + if (!Tok.is(tok::r_paren)) { + Diag(Tok.getLocation(), diag::err_attribute_wrong_number_arguments) + << AL << 1; + Parens.skipToEnd(); + return; + } + + Parens.consumeClose(); +} + +/// Acquired attributes (e.g., acquired_before, acquired_after): +/// AttrName '(' expression-list ')' +void Parser::ParseAcquiredAttribute( + 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; + } + + auto ArgStart = Tok.getLocation(); + + do { + + 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()); + + } while (TryConsumeToken(tok::comma)); + + auto ArgEnd = Tok.getLocation(); + + Attrs.addNew(&AttrName, SourceRange(ArgStart, ArgEnd), ScopeName, + ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form); + + if (EndLoc) + *EndLoc = ArgEnd; + + Parens.consumeClose(); +} + /// Bounds attributes (e.g., counted_by): /// AttrName '(' expression ')' void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName, @@ -3355,7 +3470,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..4500f17aa81df 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta %s +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -fexperimental-late-parse-attributes %s #define LOCKABLE __attribute__ ((lockable)) #define SCOPED_LOCKABLE __attribute__ ((scoped_lockable)) @@ -29,6 +29,13 @@ struct LOCKABLE Mutex {}; struct Foo { struct Mutex *mu_; + int a_value GUARDED_BY(mu_); + int* a_ptr PT_GUARDED_BY(bar.other_mu); + + struct Bar { + struct Mutex *third_mu ACQUIRED_BEFORE(other_mu); + struct Mutex *other_mu ACQUIRED_AFTER(mu_); + } bar; }; // Declare mutex lock/unlock functions. @@ -136,6 +143,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@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits