[clang] Fix GNU spellings of lifetimebound/lifetime_capture_by#192070
[clang] Fix GNU spellings of lifetimebound/lifetime_capture_by#192070
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Zeyi Xu (zeyi2) ChangesFull diff: https://github.com/llvm/llvm-project/pull/192070.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1db1d9fea6b98..3a8fe626b8ba4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -424,6 +424,8 @@ Bug Fixes to Attribute Support
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Fixed a behavioral discrepancy between deleted functions and private members when checking the ``enable_if`` attribute. (#GH175895)
- Fixed ``init_priority`` attribute by delaying type checks until after the type is deduced.
+- Fixed GNU spellings of ``lifetimebound`` and ``lifetime_capture_by`` on member function declarators to match the
+ corresponding ``[[clang::...]]`` spellings on implicit object parameters. (#GH121905)
Bug Fixes to C++ Support
^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 846474fe94adf..6d941a9db0ff7 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -734,6 +734,13 @@ static void distributeTypeAttrsFromDeclarator(TypeProcessingState &state,
continue;
switch (attr.getKind()) {
+ case ParsedAttr::AT_LifetimeBound:
+ case ParsedAttr::AT_LifetimeCaptureBy:
+ if (state.getDeclarator().isDeclarationOfFunction())
+ distributeFunctionTypeAttrFromDeclarator(state, attr, declSpecType,
+ CFT);
+ break;
+
OBJC_POINTER_TYPE_ATTRS_CASELIST:
distributeObjCPointerTypeAttrFromDeclarator(state, attr, declSpecType);
break;
@@ -9162,12 +9169,23 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type,
attr.setUsedAsTypeAttr();
break;
case ParsedAttr::AT_LifetimeBound:
- if (TAL == TAL_DeclChunk)
+ if (TAL == TAL_DeclChunk) {
HandleLifetimeBoundAttr(state, type, attr);
+ // This GNU spelling has already been consumed as a type attribute.
+ // Remove it so the later decl pass does not re-diagnose the enclosing
+ // method as subject.
+ if (!attr.isStandardAttributeSyntax() &&
+ !attr.isRegularKeywordAttribute())
+ state.getCurrentAttributes().remove(&attr);
+ }
break;
case ParsedAttr::AT_LifetimeCaptureBy:
- if (TAL == TAL_DeclChunk)
+ if (TAL == TAL_DeclChunk) {
HandleLifetimeCaptureByAttr(state, type, attr);
+ if (!attr.isStandardAttributeSyntax() &&
+ !attr.isRegularKeywordAttribute())
+ state.getCurrentAttributes().remove(&attr);
+ }
break;
case ParsedAttr::AT_OverflowBehavior:
HandleOverflowBehaviorAttr(type, attr, state);
diff --git a/clang/test/AST/attr-lifetime-capture-by.cpp b/clang/test/AST/attr-lifetime-capture-by.cpp
index 76efd45304447..21a4e46c9a446 100644
--- a/clang/test/AST/attr-lifetime-capture-by.cpp
+++ b/clang/test/AST/attr-lifetime-capture-by.cpp
@@ -8,6 +8,12 @@ struct S {
// CHECK: CXXMethodDecl {{.*}}clang::lifetime_capture_by(a, b, global)
+struct GNUCaptureBy {
+ void capture_this() __attribute__((lifetime_capture_by(global)));
+};
+
+// CHECK: CXXMethodDecl {{.*}}capture_this 'void () {{\[\[}}clang::lifetime_capture_by(global){{\]\]}}'
+
// ****************************************************************************
// Infer annotation for STL container methods.
// ****************************************************************************
diff --git a/clang/test/SemaCXX/attr-lifetime-capture-by.cpp b/clang/test/SemaCXX/attr-lifetime-capture-by.cpp
index 8606592c6b771..bad2d41722ad3 100644
--- a/clang/test/SemaCXX/attr-lifetime-capture-by.cpp
+++ b/clang/test/SemaCXX/attr-lifetime-capture-by.cpp
@@ -48,3 +48,15 @@ struct T {
void explicit_this1(this T& self, const int &x [[clang::lifetime_capture_by(self)]]);
void explicit_this2(this T& self, const int &x [[clang::lifetime_capture_by(this)]]); // expected-error {{argument references unavailable implicit 'this'}}
};
+
+struct GNUCaptureBy {
+ const int *x;
+ void captureParam(const int &v __attribute__((lifetime_capture_by(this)))) {
+ x = &v;
+ }
+ void captureThis() __attribute__((lifetime_capture_by(global)));
+};
+
+struct GNUCaptureByInvalid {
+ void member_bad() __attribute__((lifetime_capture_by())); // expected-error {{'lifetime_capture_by' attribute specifies no capturing entity}}
+};
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 9e2aaff6559c4..22a11aaafcc11 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -30,6 +30,16 @@ namespace usage_invalid {
int (X::*member_func_ptr)(int) [[clang::lifetimebound]]; // expected-error {{'clang::lifetimebound' attribute only applies to parameters and implicit object parameters}}
}
+namespace usage_invalid_gnu {
+ int *not_class_member() __attribute__((lifetimebound)); // expected-error {{non-member function has no implicit object parameter}}
+ struct A {
+ A() __attribute__((lifetimebound)); // expected-error {{cannot be applied to a constructor}}
+ ~A() __attribute__((lifetimebound)); // expected-error {{cannot be applied to a destructor}}
+ static int *static_class_member() __attribute__((lifetimebound)); // expected-error {{static member function has no implicit object parameter}}
+ void void_return_member() __attribute__((lifetimebound)); // expected-error {{'lifetimebound' attribute cannot be applied to an implicit object parameter of a function that returns void; did you mean 'lifetime_capture_by(X)'}}
+ };
+}
+
namespace usage_ok {
struct IntRef { int *target; };
@@ -96,6 +106,18 @@ namespace usage_ok {
t = {C().method()}; // expected-warning {{object backing the pointer 't' will be destroyed at the end of the full-expression}}
}
+ struct D {
+ int *method() __attribute__((lifetimebound));
+ int i;
+ };
+
+ int *D::method() { return &i; }
+
+ void test_lifetimebound_on_implicit_this_gnu() {
+ int *t = D().method(); // expected-warning {{temporary whose address is used as value of local variable 't' will be destroyed at the end of the full-expression}}
+ t = {D().method()}; // expected-warning {{object backing the pointer 't' will be destroyed at the end of the full-expression}}
+ }
+
struct FieldCheck {
struct Set {
int a;
@@ -120,6 +142,10 @@ namespace usage_ok {
const int& d = FieldCheck{x}.getNoLB()->c.a;
const int* e = FieldCheck{x}.getR().d;
}
+
+ const int &gnu_crefparam(const int ¶m __attribute__((lifetimebound)));
+ const int &gnu_crefparam(const int ¶m) { return param; }
+ const int &gnu_s = gnu_crefparam(2); // expected-warning {{temporary bound to local reference 'gnu_s' will be destroyed at the end of the full-expression}}
}
namespace std {
|
| @@ -48,3 +48,15 @@ struct T { | |||
| void explicit_this1(this T& self, const int &x [[clang::lifetime_capture_by(self)]]); | |||
| void explicit_this2(this T& self, const int &x [[clang::lifetime_capture_by(this)]]); // expected-error {{argument references unavailable implicit 'this'}} | |||
| }; | |||
|
|
|||
| struct GNUCaptureBy { | |||
There was a problem hiding this comment.
https://godbolt.org/z/j5r5joad4
This previously would give an error
| continue; | ||
|
|
||
| switch (attr.getKind()) { | ||
| case ParsedAttr::AT_LifetimeBound: |
There was a problem hiding this comment.
For an example case like this:
struct Foo2 {
int &get2() __attribute__((lifetimebound));
};before:
`-CXXMethodDecl ... get2 'int &()'
after:
`-CXXMethodDecl ... get2 'int &() [[clang::lifetimebound]]':'int &()'
isDeclarationOfFunction is used to correctly handle cases like:
const int &get3(const int &a __attribute__((lifetimebound)));Also we can't just add AT_Lifetime* to FUNCTION_TYPE_ATTRS_CASELIST, because these two already have dedicated handling in processTypeAttrs()
| @@ -734,6 +734,13 @@ static void distributeTypeAttrsFromDeclarator(TypeProcessingState &state, | |||
| continue; | |||
|
|
|||
| switch (attr.getKind()) { | |||
There was a problem hiding this comment.
I find it hard to believe that adding cases here would be the right way of doing this; I would have thought that you’d have to modify something in Attr.td to get this to work; should these attributes perhaps be DeclOrTypeAttrs?
CC @erichkeane
There was a problem hiding this comment.
Yeah, this doesn't look right to me. We're not supposed to have to do manual coverage here, the point here is that attributes that apply to the function type get attached to the function type. The fact that these AREN'T being distributed properly tells me that either this shouldn't be (which I don't think they should?) and thus adding them to the function type is wrong, or they need to be marked as FUNCTION_TYPE_ATTRS_CASELIST,
@AaronBallman probably has a better knowledge of these attributes.
There was a problem hiding this comment.
Thanks for the feedback!
I would have thought that you’d have to modify something in Attr.td to get this to work; should these attributes perhaps be DeclOrTypeAttrs?
By looking at Attr.td, it seems that they are already DeclOrTypeAttrs...
llvm-project/clang/include/clang/Basic/Attr.td
Lines 2108 to 2123 in 9f6f26f
There was a problem hiding this comment.
the point here is that attributes that apply to the function type get attached to the function type. The fact that these AREN'T being distributed properly tells me that either this shouldn't be (which I don't think they should?) and thus adding them to the function type is wrong
The behaviour of lifetimebound seems a bit subtle here: https://godbolt.org/z/Wxvnx9n35
|-TypeAliasDecl <line:1:1, col:26> col:7 referenced F 'int () [[clang::lifetimebound]]':'int ()'
| `-AttributedType 'int () [[clang::lifetimebound]]' sugar
| `-FunctionProtoType 'int ()' cdecl
| `-BuiltinType 'int'
So I think it is fair to say that lifetimebound does have a real function-type-syntax. (for some cases)
But at the same time, I do not think we should put it in FUNCTION_TYPE_ATTRS_CASELIST, cases like this won't work:
https://godbolt.org/z/W4vG1WY7d
The diagnostic there is:
error: 'clang::lifetimebound' attribute only applies to parameters and implicit object parameters
So my current understanding is that the semantic subjects of lifetimebound are still parameters / the implicit object parameter, but Clang does use a function-type representation for at least some positions today.
That's why I don't think it fits cleanly into FUNCTION_TYPE_ATTRS_CASELIST bucket, while the GNU postfix spelling still appears to need to reach the existing TAL_DeclChunk handling path.
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
I think I need more background information because the more I dig into these attributes, the less I feel like I understand them. For example, this bit from our docs already seems confused:
The attribute can be applied to the implicit
thisparameter of a member
function by writing the attribute after the function type:
That might be acceptable for __attribute__ based spellings because those do slide around to whatever makes the most sense (in general), but for [[]] based spellings, that's just not valid because it has to apply to the function type. But that might also just be a documentation bug because it seems like we do apply it to the type: https://godbolt.org/z/z5a4WKnxq (see the AST dump, we put the attribute onto the member function but not onto the type of s.) But then we allow it on a free function sometimes and I have no idea what that should do: https://godbolt.org/z/9e9qj9zx6
Btw, I also ended up filing #192102 as a drive-by when working on this review.
Another question that came to mind was: because these can be on the function type, just what impacts are there from that? Can you SFINAE based on a function type having this attribute? This sure seems like these are all supposed to be declaration attributes, not type attributes. e.g., if we had lifetime_capture_by_this as a declaration attribute there would be no need for a type attribute, right?
I did a quick experiment here: https://godbolt.org/z/8WK191dcf It seems that these attributes has a real type-syntax representation, but they do not seem to affect canonical type identity in this experiment. So at least from what I can tell, they do not appear to participate in the type system in the same way as canonical type qualifiers/attributes would. |
|
Thanks all for the feedback and the additional questions here. My original goal for this PR was fairly narrow: to fix the current GNU/standard inconsistency for the member-function postfix spelling. From the discussion so far, it seems this may be touching a broader modeling question around these attributes. So I am not sure whether this PR should try to address that design question or whether it would be preferable to keep this PR narrowly scoped to the current behavior gap and discuss the larger attribute-modeling issue separately. I would be happy to take either direction, but I wanted to check which scope would be preferred before pushing this further :) |
Thank you! It's a bit unclear what we should do here. It doesn't make much sense to redistribute to the function type if we actually want this to be handled via declaration attributes rather than type ones. Btw, the type attribute situation has another oddity that's probably a bug: https://godbolt.org/z/be6WfPEqx which is another behavior that would make more sense if this were a declaration attribute instead. |
5 participants
__attribute__((lifetimebound))and__attribute__((lifetime_capture_by(...)))on a member function declarator were not redistributed onto the function type before, so they never reachedHandleLifetimeBoundAttr/HandleLifetimeCaptureByAttrand the implicit object parameter was not annotated. This PR fixes the problem.Closes #121905