Support guarded_by attribute and related attributes inside C structs and support late parsing them
#95455
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Pierre d'Herbemont (pdherbemont) ChangesThis fixes #20777. This replaces #94216 which was reverted after being merged. Previously the 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 Full diff: https://github.com/llvm/llvm-project/pull/95455.diff 10 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8c2f737836a9d..3e6cc43652ae0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -471,6 +471,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 b70b0c8b836a5..6032b934279dd 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 4d4579fcfd456..53b5e18905ca7 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5142,7 +5142,7 @@ class Sema final : public SemaBase {
enum ExpressionKind {
EK_Decltype,
EK_TemplateArgument,
- EK_BoundsAttrArgument,
+ EK_AttrArgument,
EK_Other
} ExprContext;
@@ -5249,10 +5249,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..a8d6af2186c8a 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,118 @@ 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,
+ getLangOpts().CPlusPlus
+ ? Sema::ExpressionEvaluationContext::Unevaluated
+ : 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();
+
+ ArgsVector ArgExprs;
+
+ do {
+
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions,
+ getLangOpts().CPlusPlus
+ ? Sema::ExpressionEvaluationContext::Unevaluated
+ : 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 +3477,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 99a8704298314..2c0d9d75fcc8c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -2719,11 +2719,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.
@@ -3357,7 +3356,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
index 642ea88ec3c96..50934f5476157 100644
--- 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_);
+ 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)));
+ };
};
// 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
index 73cc946ca0ce1..af9254508d805 100644
--- 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() {
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 0c5b0cc85897b..462cd2437814b 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -1517,10 +1517,10 @@ class Foo {
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
+ //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
+ #endif
static void foo() EXCLUSIVE_LOCKS_REQUIRED(mu);
//FIXME: Bug 32066 - Error should be emitted irrespective of C++ dialect
|
hnrklssn
reviewed
Jun 14, 2024
clang/lib/Parse/ParseDecl.cpp
Outdated
| Actions, | ||
| getLangOpts().CPlusPlus | ||
| ? Sema::ExpressionEvaluationContext::Unevaluated | ||
| : Sema::ExpressionEvaluationContext::PotentiallyEvaluated, |
There was a problem hiding this comment.
Why does C need to parse using PotentiallyEvaluated while C++ needs Unevaluated? What happens if we parse using Unevaluted in C?
There was a problem hiding this comment.
I thought it wasn't working properly with Unevaluated but it seems like it actually does!
I have iterated on the patch further and simplified a bit by directly passing EK_AttrArgument to EnterExpressionEvaluationContext from within ParseAttributeArgsCommon. This allows me to remove ParseGuardedAttribute() and ParseAcquiredAttribute() and only rely on ParseAttributeArgsCommon.
This may have unforeseen side effects on some GNU/Clang/Microsoft attributes, but those are not caught by the test suite... So I am tempted to propose the simpler patch.
pdherbemont
force-pushed
the
main
branch
3 times, most recently
from
db25141 to
8bfc79e
Compare
rapidsna
reviewed
Jun 18, 2024
|
|
||
| #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__))) |
There was a problem hiding this comment.
What's the reason to change it to take va_arg?
There was a problem hiding this comment.
To be able to test passing multiple arguments. In the previous iteration of the patch, the attribute was parsed manually and we needed to make sure all cases were covered.
|
I think this still needs review from @delcypher and @rapidsna |
aaronpuchert
reviewed
Jul 1, 2024
| int a_value GUARDED_BY(mu_); | ||
|
|
||
| struct Bar { | ||
| struct Mutex *other_mu ACQUIRED_AFTER(mu_); |
There was a problem hiding this comment.
I'm not entirely sure how nested structs work in C, but should we be able to reference the parent from the child? At least in C++, you can declare objects of the child independent of the parent, so this wouldn't be meaningful.
This is an existing issue with with the ACQUIRED_{AFTER,BEFORE} implementation, but if you add a test for this (which is Ok to make sure it doesn't crash), you should probably also add a comment that this should perhaps be disallowed because it's not meaningful (as I understand).
There was a problem hiding this comment.
I'm not entirely sure how nested structs work in C, but should we be able to reference the parent from the child?
I think so, it does read nicely it feels – I see why this could cause a problem though: if the child structure is used outside of the parent, there is an ambiguity. Maybe this should be restricted to anonymous struct? Or maybe we need to taint the child struct so that it's not reused outside of the parent?
Anyhow, I'll add a comment as suggested!
There was a problem hiding this comment.
@pdherbemont Did you add the comment? I can't find it in the diff for this file as shown on GitHub
| struct A { | ||
| struct Lock lock; | ||
| union { | ||
| int b __attribute__((guarded_by(lock))); |
There was a problem hiding this comment.
Tricky, see #78131.
But as above, testing that it doesn't crash is fine.
pdherbemont
force-pushed
the
main
branch
2 times, most recently
from
4edb519 to
2a8eb78
Compare
rapidsna
approved these changes
Jul 6, 2024
Yes, please! |
delcypher
approved these changes
Jul 8, 2024
There was a problem hiding this comment.
@pdherbemont Thanks for working on this LGTM. Please confirm that the comments that @aaronpuchert wanted have been added because I couldn't see them.
Once you've done that we can get this merged :)
…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; }; ```
Done! |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…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; }; ```
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #20777. This replaces #94216 which was reverted after being merged.
Previously the
guarded_by,pt_guarded_by,acquired_after, andacquired_beforeattributes 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.