Skip to content

[SYCL] IsDerivedFromInclusive expects a compelete type in the presence of attribute add_ir_attributes_global_variable. #15897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 13, 2024
Merged

[SYCL] IsDerivedFromInclusive expects a compelete type in the presence of attribute add_ir_attributes_global_variable. #15897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/lib/Sema/SemaAccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ static AccessResult IsDerivedFromInclusive(const CXXRecordDecl *Derived,
const CXXRecordDecl *Target) {
assert(Derived->getCanonicalDecl() == Derived);
assert(Target->getCanonicalDecl() == Target);
assert(Derived->getDefinition() && "Expecting a complete type");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is because types need to be complete to determine inheritance hierarchy? @AaronBallman can you confirm this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.


if (Derived == Target) return AR_accessible;

Expand Down Expand Up @@ -776,6 +777,8 @@ static AccessResult HasAccess(Sema &S,
// [B3] and [M3]
} else {
assert(Access == AS_protected);
if (!ECRecord->getDefinition())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these changes be made in community clang? This doesn't seem SYCL specific

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried but I there is no upstream class attribute that accepts an arbitrary expression. Couldn't find a test case that crashes. The closest one I could find is https://godbolt.org/z/qxEjjvf73 , but it doesn't crash. It returns the expected error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is ECRecord not defined here? And why is this restriction only required for protected access?

Copy link
Contributor Author

@zahiraam zahiraam Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is ECRecord not defined here?

This function is checking if the entity is accessible with the specific AccessSpecifier. If it's protected then we need to make sure that the type is complete. A type is incomplete if !getDefinition() .

When is ECRecord not defined here? And why is this restriction only required for protected access?

Please see comments at line 858 concerning [M3] and [B3].
IsDerivedFromInclusive checks whether a class is derived from another class. The question is, should IsDerivedFromInclusive have a precondition on Derived being a complete type. I think it should. See https://eel.is/c++draft/class.derived 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://eel.is/c++draft/class.access [M3] and https://eel.is/c++draft/class.access#class.protected [B3]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining.

continue;
switch (IsDerivedFromInclusive(ECRecord, NamingClass)) {
case AR_accessible: break;
case AR_inaccessible: continue;
Expand Down
13 changes: 13 additions & 0 deletions clang/test/SemaSYCL/attr-add-ir-attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,3 +991,16 @@ struct __attribute__((sycl_special_class)) InvalidSpecialClassStruct32 {
struct [[__sycl_detail__::add_ir_attributes_kernel_parameter("Attr1", 1)]] InvalidKernelParameterSubjectStruct; // expected-error {{'add_ir_attributes_kernel_parameter' attribute only applies to parameters}}
[[__sycl_detail__::add_ir_attributes_kernel_parameter("Attr1", 1)]] void InvalidKernelParameterSubjectFunction() {} // expected-error {{'add_ir_attributes_kernel_parameter' attribute only applies to parameters}}
[[__sycl_detail__::add_ir_attributes_kernel_parameter("Attr1", 1)]] int InvalidKernelParameterSubjectVar; // expected-error {{'add_ir_attributes_kernel_parameter' attribute only applies to parameters}}

struct A {
protected:
static constexpr const char *ir_attribute_name = ""; // expected-note {{declared protected here}}
static constexpr auto ir_attribute_value = nullptr; // expected-note {{declared protected here}}
};

template <typename Ts>
struct [[__sycl_detail__::add_ir_attributes_global_variable(
Ts::ir_attribute_name, Ts::ir_attribute_value)]] B { // expected-error {{'ir_attribute_name' is a protected member of 'A'}} // expected-error {{'ir_attribute_value' is a protected member of 'A'}}
};

B<A> v; // expected-note {{in instantiation of template class 'B<A>' requested here}}
Loading