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

[zahiraam]

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.

[elizabethandrews]

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

[zahiraam]

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.

[elizabethandrews]

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

[zahiraam]

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.

[zahiraam]

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

[elizabethandrews]

Thanks for explaining.

[zahiraam]

ping?

[elizabethandrews]

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

[AaronBallman]

That's correct.

[elizabethandrews]

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

[zahiraam]

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]

Thanks for explaining.

[zahiraam]

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