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

pdherbemont · aaryanshukla · commit 35816d42e714 · 2024-07-14T17:59:37.000Z
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; }; ```
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -549,6 +549,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``.
 
 - Introduced new function type attributes ``[[clang::nonblocking]]``, ``[[clang::nonallocating]]``,
   ``[[clang::blocking]]``, and ``[[clang::allocating]]``, with GNU-style variants as well.
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
@@ -764,10 +764,11 @@ 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.
--------------------------------------------------------------------------
+ACQUIRED_BEFORE(...) and ACQUIRED_AFTER(...) support is still experimental.
+---------------------------------------------------------------------------
 
-To be fixed in a future update.
+ACQUIRED_BEFORE(...) and ACQUIRED_AFTER(...) are currently being developed under
+the ``-Wthread-safety-beta`` flag.
 
 
 .. _mutexheader:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
@@ -3660,7 +3660,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;
@@ -3671,7 +3671,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;
@@ -3682,7 +3682,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;
@@ -3693,7 +3693,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/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -6341,7 +6341,7 @@ class Sema final : public SemaBase {
     enum ExpressionKind {
       EK_Decltype,
       EK_TemplateArgument,
-      EK_BoundsAttrArgument,
+      EK_AttrArgument,
       EK_Other
     } ExprContext;
 
@@ -6454,10 +6454,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
@@ -593,7 +593,9 @@ unsigned Parser::ParseAttributeArgsCommon(
           EnterExpressionEvaluationContext Unevaluated(
               Actions,
               Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
-                     : Sema::ExpressionEvaluationContext::ConstantEvaluated);
+                     : Sema::ExpressionEvaluationContext::ConstantEvaluated,
+              nullptr,
+              Sema::ExpressionEvaluationContextRecord::EK_AttrArgument);
 
           ExprResult ArgExpr(
               Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
@@ -610,9 +612,12 @@ unsigned Parser::ParseAttributeArgsCommon(
       // General case. Parse all available expressions.
       bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
       EnterExpressionEvaluationContext Unevaluated(
-          Actions, Uneval
-                       ? Sema::ExpressionEvaluationContext::Unevaluated
-                       : Sema::ExpressionEvaluationContext::ConstantEvaluated);
+          Actions,
+          Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
+                 : Sema::ExpressionEvaluationContext::ConstantEvaluated,
+          nullptr,
+          Sema::ExpressionEvaluationContextRecord::ExpressionKind::
+              EK_AttrArgument);
 
       ExprVector ParsedExprs;
       ParsedAttributeArgumentsProperties ArgProperties =
@@ -3383,7 +3388,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
@@ -2666,11 +2666,10 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
     return ExprError();
   }
 
-  // BoundsSafety: This specially handles arguments of bounds attributes
-  // 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()) {
+  // This specially handles arguments of attributes 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 (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.
@@ -3279,7 +3278,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/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -1,10 +1,11 @@
 // 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 -DLATE_PARSING %s
 
 #define LOCKABLE            __attribute__ ((lockable))
 #define SCOPED_LOCKABLE     __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x)       __attribute__ ((guarded_by(x)))
+#define GUARDED_BY(...)     __attribute__ ((guarded_by(__VA_ARGS__)))
 #define GUARDED_VAR         __attribute__ ((guarded_var))
-#define PT_GUARDED_BY(x)    __attribute__ ((pt_guarded_by(x)))
+#define PT_GUARDED_BY(...)  __attribute__ ((pt_guarded_by(__VA_ARGS__)))
 #define PT_GUARDED_VAR      __attribute__ ((pt_guarded_var))
 #define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__)))
 #define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__)))
@@ -29,6 +30,22 @@ struct LOCKABLE Mutex {};
 
 struct Foo {
   struct Mutex *mu_;
+  int  a_value GUARDED_BY(mu_);
+
+  struct Bar {
+    struct Mutex *other_mu ACQUIRED_AFTER(mu_); // Note: referencing the parent structure is convenient here, but this should probably be disallowed if the child structure is re-used outside of the parent.
+    struct Mutex *third_mu ACQUIRED_BEFORE(other_mu);
+  } bar;
+
+  int* a_ptr PT_GUARDED_BY(bar.other_mu);
+};
+
+struct LOCKABLE Lock {};
+struct A {
+        struct Lock lock;
+        union {
+                int b __attribute__((guarded_by(lock))); // Note: referencing the parent structure is convenient here, but this should probably be disallowed if the child is re-used outside of the parent.
+        };
 };
 
 // Declare mutex lock/unlock functions.
@@ -74,6 +91,19 @@ int get_value(int *p) SHARED_LOCKS_REQUIRED(foo_.mu_){
 
 void unlock_scope(struct Mutex *const *mu) __attribute__((release_capability(**mu)));
 
+// Verify late parsing:
+#ifdef LATE_PARSING
+struct LateParsing {
+  int a_value_defined_before GUARDED_BY(a_mutex_defined_late);
+  int *a_ptr_defined_before PT_GUARDED_BY(a_mutex_defined_late);
+  struct Mutex *a_mutex_defined_early
+    ACQUIRED_BEFORE(a_mutex_defined_late);
+  struct Mutex *a_mutex_defined_late
+    ACQUIRED_AFTER(a_mutex_defined_very_late);
+  struct Mutex *a_mutex_defined_very_late;
+} late_parsing;
+#endif
+
 int main(void) {
 
   Foo_fun1(1); // expected-warning{{calling function 'Foo_fun1' requires holding mutex 'mu2'}} \
@@ -136,9 +166,51 @@ 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);
+
+#ifdef LATE_PARSING
+  late_parsing.a_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late' exclusively}}
+  late_parsing.a_ptr_defined_before = 0;
+  mutex_exclusive_lock(late_parsing.a_mutex_defined_late);
+  mutex_exclusive_lock(late_parsing.a_mutex_defined_early); // expected-warning{{mutex 'a_mutex_defined_early' must be acquired before 'a_mutex_defined_late'}}
+  mutex_exclusive_unlock(late_parsing.a_mutex_defined_early);
+  mutex_exclusive_unlock(late_parsing.a_mutex_defined_late);
+  mutex_exclusive_lock(late_parsing.a_mutex_defined_late);
+  mutex_exclusive_lock(late_parsing.a_mutex_defined_very_late); // expected-warning{{mutex 'a_mutex_defined_very_late' must be acquired before 'a_mutex_defined_late'}}
+  mutex_exclusive_unlock(late_parsing.a_mutex_defined_very_late);
+  mutex_exclusive_unlock(late_parsing.a_mutex_defined_late);
+#endif
+
   return 0;
 }
 
 // We had a problem where we'd skip all attributes that follow a late-parsed
 // attribute in a single __attribute__.
 void run(void) __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}}
+
+int value_with_wrong_number_of_args GUARDED_BY(mu1, mu2); // expected-error{{'guarded_by' attribute takes one argument}}
+
+int *ptr_with_wrong_number_of_args PT_GUARDED_BY(mu1, mu2); // expected-error{{'pt_guarded_by' attribute takes one argument}}
+
+int value_with_no_open_brace __attribute__((guarded_by)); // expected-error{{'guarded_by' attribute takes one argument}}
+int *ptr_with_no_open_brace __attribute__((pt_guarded_by)); // expected-error{{'pt_guarded_by' attribute takes one argument}}
+
+int value_with_no_open_brace_on_acquire_after __attribute__((acquired_after)); // expected-error{{'acquired_after' attribute takes at least 1 argument}}
+int value_with_no_open_brace_on_acquire_before __attribute__((acquired_before)); // expected-error{{'acquired_before' attribute takes at least 1 argument}}
+
+int value_with_bad_expr GUARDED_BY(bad_expr); // expected-error{{use of undeclared identifier 'bad_expr'}}
+int *ptr_with_bad_expr PT_GUARDED_BY(bad_expr); // expected-error{{use of undeclared identifier 'bad_expr'}}
+
+int value_with_bad_expr_on_acquire_after __attribute__((acquired_after(other_bad_expr))); //  expected-error{{use of undeclared identifier 'other_bad_expr'}}
+int value_with_bad_expr_on_acquire_before __attribute__((acquired_before(other_bad_expr))); //  expected-error{{use of undeclared identifier 'other_bad_expr'}}
+
+int a_final_expression = 0;
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -141,6 +141,36 @@ class MutexWrapper {
    void MyLock() EXCLUSIVE_LOCK_FUNCTION(mu);
 };
 
+struct TestingMoreComplexAttributes {
+   Mutex lock;
+   struct { Mutex lock; } strct;
+   union {
+       bool a __attribute__((guarded_by(lock)));
+       bool b __attribute__((guarded_by(strct.lock)));
+       bool *ptr_a __attribute__((pt_guarded_by(lock)));
+       bool *ptr_b __attribute__((pt_guarded_by(strct.lock)));
+       Mutex lock1 __attribute__((acquired_before(lock))) __attribute__((acquired_before(strct.lock)));
+       Mutex lock2 __attribute__((acquired_after(lock))) __attribute__((acquired_after(strct.lock)));
+   };
+} more_complex_atttributes;
+
+void more_complex_attributes() {
+    more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'lock' exclusively}}
+    more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'strct.lock' exclusively}}
+    *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'lock' exclusively}}
+    *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'strct.lock' exclusively}}
+
+    more_complex_atttributes.lock.Lock();
+    more_complex_atttributes.lock1.Lock(); // expected-warning{{mutex 'lock1' must be acquired before 'lock'}}
+    more_complex_atttributes.lock1.Unlock();
+    more_complex_atttributes.lock.Unlock();
+
+    more_complex_atttributes.lock2.Lock();
+    more_complex_atttributes.lock.Lock(); // expected-warning{{mutex 'lock' must be acquired before 'lock2'}}
+    more_complex_atttributes.lock.Unlock();
+    more_complex_atttributes.lock2.Unlock();
+}
+
 MutexWrapper sls_mw;
 
 void sls_fun_0() {
