Support [[guarded_by(mutex)]] attribute inside C struct #94216

pdherbemont · 2024-06-03T13:22:04Z

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.

github-actions · 2024-06-03T13:22:24Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-03T13:22:55Z

@llvm/pr-subscribers-clang

Author: Pierre d'Herbemont (pdherbemont)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/94216.diff

4 Files Affected:

(modified) clang/include/clang/Parse/Parser.h (+6)
(modified) clang/lib/Parse/ParseDecl.cpp (+59)
(modified) clang/test/Sema/warn-thread-safety-analysis.c (+9-1)
(modified) clang/test/SemaCXX/warn-thread-safety-parsing.cpp (+1-5)

diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index d054b8cf0d240..308f6ea121eab 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3128,6 +3128,12 @@ class Parser : public CodeCompletionHandler {
                             IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
                             ParsedAttr::Form Form);
 
+  void ParseGuardedByAttribute(IdentifierInfo &AttrName,
+                               SourceLocation AttrNameLoc,
+                               ParsedAttributes &Attrs,
+                               IdentifierInfo *ScopeName,
+                               SourceLocation ScopeLoc, ParsedAttr::Form Form);
+
   void ParseTypeofSpecifier(DeclSpec &DS);
   SourceLocation ParseDecltypeSpecifier(DeclSpec &DS);
   void AnnotateExistingDecltypeSpecifier(const DeclSpec &DS,
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index c528917437332..58a3de7210455 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -671,6 +671,14 @@ void Parser::ParseGNUAttributeArgs(
     ParseBoundsAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
                          Form);
     return;
+  } else if (AttrKind == ParsedAttr::AT_GuardedBy) {
+    ParseGuardedByAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
+                            Form);
+    return;
+  } else if (AttrKind == ParsedAttr::AT_PtGuardedBy) {
+    ParseGuardedByAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
+                            Form);
+    return;
   } else if (AttrKind == ParsedAttr::AT_CXXAssume) {
     ParseCXXAssumeAttributeArg(Attrs, AttrName, AttrNameLoc, EndLoc, Form);
     return;
@@ -3330,6 +3338,57 @@ 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,
+                                     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_BoundsAttrArgument);
+
+  ExprResult ArgExpr(
+      Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+  if (ArgExpr.isInvalid()) {
+    Parens.skipToEnd();
+    return;
+  }
+
+  ArgExprs.push_back(ArgExpr.get());
+
+  auto &AL = *Attrs.addNew(
+      &AttrName, SourceRange(AttrNameLoc, Parens.getCloseLocation()), ScopeName,
+      ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form);
+
+  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,
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 642ea88ec3c96..abad65b0b9154 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -28,7 +28,12 @@
 struct LOCKABLE Mutex {};
 
 struct Foo {
-  struct Mutex *mu_;
+    struct Mutex *mu_;
+    struct Bar {
+        struct Mutex *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 +141,9 @@ 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}}
+
   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

clang/lib/Parse/ParseDecl.cpp

AaronBallman

Thank you for working on this!

The changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the new functionality.

One question I have is whether there will be follow-up work for other thread safety attributes (acquire_capability, try_acquire_capability, etc)?

clang/lib/Parse/ParseDecl.cpp

rapidsna

You may also want to consider making the attribute late parsed in C when -fexperimental-late-parse-attributes is enabled. See https://github.com/llvm/llvm-project/pull/93121/files#diff-ae2ec9524bdbeea1f06917607482634dd89af5bcbb929805032463e5dafe79e7R2260

That will allow the code like below:

 struct Foo {
   int a_value GUARDED_BY(mu_); // attribute comes before `mu_` which needs to be late parsed
   struct Mutext *mu_;
 }

clang/lib/Parse/ParseDecl.cpp

clang/test/Sema/warn-thread-safety-analysis.c

pdherbemont · 2024-06-05T13:32:32Z

One question I have is whether there will be follow-up work for other thread safety attributes (acquire_capability, try_acquire_capability, etc)?

They already work fine in C so far. But if you are aware of some issues don't hesitate to let me know!

Endilll

Sema.h changes look good to me.

AaronBallman · 2024-06-05T14:08:32Z

One question I have is whether there will be follow-up work for other thread safety attributes (acquire_capability, try_acquire_capability, etc)?

They already work fine in C so far. But if you are aware of some issues don't hesitate to let me know!

Ah, sorry, of course I picked examples that appertain to functions rather than fields. But some of the other thread safety attributes have the same problem as guarded_by: https://godbolt.org/z/hWTMxzrns

pdherbemont · 2024-06-05T14:09:02Z

You may also want to consider making the attribute late parsed in C when -fexperimental-late-parse-attributes is enabled. See https://github.com/llvm/llvm-project/pull/93121/files#diff-ae2ec9524bdbeea1f06917607482634dd89af5bcbb929805032463e5dafe79e7R2260

That will allow the code like below:
 struct Foo {
   int a_value GUARDED_BY(mu_); // attribute comes before `mu_` which needs to be late parsed
   struct Mutext *mu_;
 }

Adopted LateAttrParseExperimentalExt. Let me know if that looks okay.

pdherbemont · 2024-06-05T14:11:30Z

One question I have is whether there will be follow-up work for other thread safety attributes (acquire_capability, try_acquire_capability, etc)?

They already work fine in C so far. But if you are aware of some issues don't hesitate to let me know!

Ah, sorry, of course I picked examples that appertain to functions rather than fields. But some of the other thread safety attributes have the same problem as guarded_by: https://godbolt.org/z/hWTMxzrns

Oh yes – maybe it makes sense to fix them in this commit. Let me try to look into that quickly.

delcypher · 2024-06-12T18:21:14Z

@pdherbemont I've landed on your behalf. I tweaked the commit message to be more descriptive of the change we finally landed and noted that you are the author of this patch. Thanks again for working on this :)

aeubanks · 2024-06-12T22:31:13Z

this seems to have broken code like this with

../../base/allocator/partition_allocator/src/partition_alloc/random.cc:33:69: error: invalid use of non-static data member 'lock_'
   33 |     internal::base::InsecureRandomGenerator instance_ PA_GUARDED_BY(lock_);
      |                                                                     ^~~~~
../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/thread_annotations.h:59:70: note: expanded from macro 'PA_GUARDED_BY'
   59 | #define PA_GUARDED_BY(x) PA_THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
      |                                                                      ^
../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/thread_annotations.h:44:60: note: expanded from macro 'PA_THREAD_ANNOTATION_ATTRIBUTE__'
   44 | #define PA_THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))
      |                                                            ^
../../base/allocator/partition_allocator/src/partition_alloc/random.cc:35:65: error: invalid use of non-static data member 'lock_'
   35 |         internal::base::InsecureRandomGenerator)] PA_GUARDED_BY(lock_) = {};
      |                                                                 ^~~~~
../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/thread_annotations.h:59:70: note: expanded from macro 'PA_GUARDED_BY'
   59 | #define PA_GUARDED_BY(x) PA_THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
      |                                                                      ^
../../base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/thread_annotations.h:44:60: note: expanded from macro 'PA_THREAD_ANNOTATION_ATTRIBUTE__'
   44 | #define PA_THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))
      |                                                            ^

is that intentional?

aeubanks · 2024-06-12T22:39:22Z

reduced:

struct Lock {};
struct A {
        Lock lock;
        union {
                bool b __attribute__((guarded_by(lock)));
        };
};

seems like a regression, can we revert for now?

delcypher · 2024-06-12T22:41:26Z

@aeubanks I'll revert. Is this example C or C++?

…C structs and support late parsing them (#94216)" This reverts commit af0d712. Reverting due to likely regression: #94216 (comment)

delcypher · 2024-06-12T22:44:30Z

Reverted in c9d5800

delcypher · 2024-06-12T22:45:07Z

@pdherbemont Could you take a look at this?

aeubanks · 2024-06-13T02:05:33Z

@aeubanks I'll revert. Is this example C or C++?

C++

pdherbemont · 2024-06-13T08:17:57Z

Ah nice catch! Thank you @aeubanks and @delcypher – I am going to try to understand why (and add a new test)

pdherbemont · 2024-06-13T14:13:54Z

So I found the cause of the regression: We used to parse the argument in an Unevaluated context in C++ mode. We now parse them in a PotentiallyEvaluated context. Switching to Unevaluated makes the C parsing code fail. A simple fix is to switch the context based on the language detected.

…s and support late parsing them This fixes llvm#20777. This replaces llvm#94216 which was reverted after being merged. Previously the `guarded_by`, `pt_guarded_by`, `acquired_after`, and `acquired_before` attributes were only supported inside C++ classes or top level C/C++ declaration. This patch allows these attributes to be added to struct members in C. These attributes have also now support experimental late parsing. This is off by default but can be enabled by passing `-fexperimental-late-parse-attributes`. This is useful for referring to a struct member after the annotated member. E.g. ``` struct Example { int a_value_defined_before __attribute__ ((guarded_by(a_mutex))); struct Mutex *a_mutex; }; ```

pdherbemont · 2024-06-13T19:20:56Z

Follow-up PR #95455

…s and support late parsing them This fixes llvm#20777. This replaces llvm#94216 which was reverted after being merged. Previously the `guarded_by`, `pt_guarded_by`, `acquired_after`, and `acquired_before` attributes were only supported inside C++ classes or top level C/C++ declaration. This patch allows these attributes to be added to struct members in C. These attributes have also now support experimental late parsing. This is off by default but can be enabled by passing `-fexperimental-late-parse-attributes`. This is useful for referring to a struct member after the annotated member. E.g. ``` struct Example { int a_value_defined_before __attribute__ ((guarded_by(a_mutex))); struct Mutex *a_mutex; }; ```

…s and support late parsing them (#95455) This fixes #20777. This replaces #94216 which was reverted after being merged. Previously the `guarded_by`, `pt_guarded_by`, `acquired_after`, and `acquired_before` attributes were only supported inside C++ classes or top level C/C++ declaration. This patch allows these attributes to be added to struct members in C. These attributes have also now support experimental late parsing. This is off by default but can be enabled by passing `-fexperimental-late-parse-attributes`. This is useful for referring to a struct member after the annotated member. E.g. ``` struct Example { int a_value_defined_before __attribute__ ((guarded_by(a_mutex))); struct Mutex *a_mutex; }; ```

…s and support late parsing them (llvm#95455) This fixes llvm#20777. This replaces llvm#94216 which was reverted after being merged. Previously the `guarded_by`, `pt_guarded_by`, `acquired_after`, and `acquired_before` attributes were only supported inside C++ classes or top level C/C++ declaration. This patch allows these attributes to be added to struct members in C. These attributes have also now support experimental late parsing. This is off by default but can be enabled by passing `-fexperimental-late-parse-attributes`. This is useful for referring to a struct member after the annotated member. E.g. ``` struct Example { int a_value_defined_before __attribute__ ((guarded_by(a_mutex))); struct Mutex *a_mutex; }; ```

bvanassche · 2025-01-14T18:32:36Z

Thread-safety attributes of cleanup functions are ignored. Does anyone know how to fix this? See also #122758.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 3, 2024

pdherbemont force-pushed the main branch 2 times, most recently from ccf6699 to bf9a2ba Compare June 4, 2024 13:50

rapidsna requested review from rapidsna, delcypher, AaronBallman, erichkeane, hnrklssn and patrykstefanski June 4, 2024 15:38

pdherbemont force-pushed the main branch from bf9a2ba to 3cfee20 Compare June 4, 2024 15:40

pdherbemont commented Jun 4, 2024

View reviewed changes

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

erichkeane requested review from faisalv, dwblaikie and tbaederr June 4, 2024 15:42

AaronBallman requested review from aaronpuchert and removed request for faisalv June 4, 2024 16:23

AaronBallman reviewed Jun 4, 2024

View reviewed changes

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

rapidsna reviewed Jun 4, 2024

View reviewed changes

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

rapidsna reviewed Jun 4, 2024

View reviewed changes

clang/test/Sema/warn-thread-safety-analysis.c Show resolved Hide resolved

pdherbemont force-pushed the main branch from 3cfee20 to 0fd6864 Compare June 5, 2024 13:58

pdherbemont requested a review from Endilll as a code owner June 5, 2024 13:58

pdherbemont force-pushed the main branch from 0fd6864 to 7d01e8a Compare June 5, 2024 14:00

Endilll reviewed Jun 5, 2024

View reviewed changes

delcypher mentioned this pull request Jun 12, 2024

Thread Safety Analysis "guarded_by" attribute doesn't work for struct fields in C code #20777

Closed

delcypher added a commit that referenced this pull request Jun 12, 2024

Revert "Support guarded_by attribute and related attributes inside …

c9d5800

…C structs and support late parsing them (#94216)" This reverts commit af0d712. Reverting due to likely regression: #94216 (comment)

pdherbemont mentioned this pull request Jun 13, 2024

Support guarded_by attribute and related attributes inside C structs and support late parsing them #95455

Merged

Support [[guarded_by(mutex)]] attribute inside C struct #94216

Support [[guarded_by(mutex)]] attribute inside C struct #94216

Uh oh!

Conversation

pdherbemont commented Jun 3, 2024

Uh oh!

github-actions bot commented Jun 3, 2024

Uh oh!

llvmbot commented Jun 3, 2024

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rapidsna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pdherbemont commented Jun 5, 2024

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Jun 5, 2024

Uh oh!

pdherbemont commented Jun 5, 2024

Uh oh!

pdherbemont commented Jun 5, 2024

Uh oh!

delcypher commented Jun 12, 2024

Uh oh!

aeubanks commented Jun 12, 2024

Uh oh!

aeubanks commented Jun 12, 2024

Uh oh!

delcypher commented Jun 12, 2024

Uh oh!

delcypher commented Jun 12, 2024

Uh oh!

delcypher commented Jun 12, 2024

Uh oh!

aeubanks commented Jun 13, 2024

Uh oh!

pdherbemont commented Jun 13, 2024

Uh oh!

pdherbemont commented Jun 13, 2024

Uh oh!

pdherbemont commented Jun 13, 2024

Uh oh!

bvanassche commented Jan 14, 2025

Uh oh!

Uh oh!