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

Jump to bottom
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

merged 1 commit into from
Nov 13, 2024

Conversation

zahiraam
Copy link
Contributor

@zahiraam zahiraam commented Oct 28, 2024 •
edited
Loading

This test is crashing the compiler.
struct A {
protected:
static constexpr const char *ir_attribute_name = "";
static constexpr auto ir_attribute_value = nullptr;
};

template <typename Ts>
struct [[__sycl_detail__::add_ir_attributes_global_variable(
Ts::ir_attribute_name, Ts::ir_attribute_value)]] B {
};
B<A> v;

When a class attribute accepts an arbitrary expression, the underlying class needs to be a complete type.

@zahiraam zahiraam changed the title IsDerivedFromInclusive expects a compelete type. [SYCL] sDerivedFromInclusive expects a compelete type with add_ir_attributes_global_variable. Oct 28, 2024
@zahiraam zahiraam changed the title [SYCL] sDerivedFromInclusive expects a compelete type with add_ir_attributes_global_variable. [SYCL] IsDerivedFromInclusive expects a compelete type with add_ir_attributes_global_variable. Oct 28, 2024
@zahiraam zahiraam changed the title [SYCL] IsDerivedFromInclusive expects a compelete type with add_ir_attributes_global_variable. [SYCL] IsDerivedFromInclusive expects a compelete type in the presence of attribute add_ir_attributes_global_variable. Oct 28, 2024
@zahiraam zahiraam marked this pull request as ready for review October 29, 2024 12:01
@zahiraam zahiraam requested a review from a team as a code owner October 29, 2024 12:01
elizabethandrews
elizabethandrews reviewed Oct 31, 2024
View reviewed changes
clang/lib/Sema/SemaAccess.cpp
@@ -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.

@zahiraam
Copy link
Contributor Author

zahiraam commented Nov 5, 2024

ping?

elizabethandrews
elizabethandrews reviewed Nov 5, 2024
View reviewed changes
clang/lib/Sema/SemaAccess.cpp
@@ -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.

clang/lib/Sema/SemaAccess.cpp
@@ -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.

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

@zahiraam
Copy link
Contributor Author

IsDrivedfromInclusive is called with an effective context that’s a Record. In this function there is a call to Derived->bases() that returns null in the context of this specific test case.
In clang there are attributes that produce the expected diagnostic, but they don’t exhibit the crash that this test case does. https://godbolt.org/z/bsqxozvdx and the test case in the patch follow the same code path, but in the first case the call to Derived->bases() returns something because its DefinitionData is non-null (type is complete), whereas in the second case DefinitionData is null (type in not complete). This patch is ensuring that the call to IsDrivedfromInclusive happens only when the type is complete. It’s not changing the inheritance rules.

elizabethandrews
elizabethandrews approved these changes Nov 12, 2024
View reviewed changes
clang/lib/Sema/SemaAccess.cpp
@@ -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.

Thanks for explaining.

@zahiraam
Copy link
Contributor Author

@intel/llvm-gatekeepers Can this be merged in please? Thanks.

@sarnex sarnex merged commit 0a3a324 into intel:sycl Nov 13, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronBallman AaronBallman AaronBallman left review comments

@elizabethandrews elizabethandrews elizabethandrews approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zahiraam @AaronBallman @elizabethandrews @sarnex