c++: __builtin_is_pointer_interconvertible_with_class incremental fix [PR101539]

On Thu, Jul 29, 2021 at 09:50:10AM +0200, Jakub Jelinek via Gcc-patches wrote:
> Now that I'm writing the above text and rereading the
> pointer-interconvertibility definition, I think my first_nonstatic_data_member_p
> and fold_builtin_is_pointer_inverconvertible_with_class have one bug,
> for unions the pointer inter-convertibility doesn't talk about std layout at
> all, so I think I need to check for std_layout_type_p only for non-union
> class types and accept any union, std_layout_type_p or not.  But when
> recursing from a union type into anonymous structure type punt if the
> anonymous structure type is not std_layout_type_p + add testcase coverage.

For this part, here is an incremental fix.  Tested on x86_64-linux.

It also shows that in the case (we're beyond the standard in this case
because anonymous structures are not in the standard) of union with
non-std-layout anonymous structure in it, in the case in the testcases like:
struct D {};
struct E { [[no_unique_address]] D e; };
union Y { int a; struct : public E { short b; long c; }; long long d; };
the builtin will return false for &Y::b - while &Y::b is at offset zero,
the anonymous structure is not std-layout and therefore the
pointer-interconvertibility rules say pointers aren't interconvertible.
But in case like:
union Y2 { int a; struct : public E { int b; long c; }; long long d; };
it will return true for &Y2::b - while the same applies, there is
another union member with int type.  In theory when seeing the PTRMEM_CST
we could still differentiate, &Y2::a is ok but &Y2::b is not.  But as soon
as we have just an INTEGER_CST with OFFSET_TYPE or need to check it at
runtime, all we know is that we have pointer to int data member in Y2
at offset 0, and that is the same for &Y2::a and &Y2::b.

2021-07-29  Jakub Jelinek  <jakub@redhat.com>

	PR c++/101539
	* semantics.c (first_nonstatic_data_member_p): Don't recurse into
	non-std-layout non-union class types from union type.
	(fold_builtin_is_pointer_inverconvertible_with_class): Don't check
	std-layout type for union types.

	* g++.dg/cpp2a/is-pointer-interconvertible-with-class1.C: Add
	tests for non-std-layout union type.
	* g++.dg/cpp2a/is-pointer-interconvertible-with-class2.C: Likewise.
	* g++.dg/cpp2a/is-pointer-interconvertible-with-class4.C: Add
	tests for non-std-layout anonymous class type in union.
	* g++.dg/cpp2a/is-pointer-interconvertible-with-class5.C: Likewise.


	Jakub

On 7/29/21 10:21 AM, Jakub Jelinek wrote:
> On Thu, Jul 29, 2021 at 09:50:10AM +0200, Jakub Jelinek via Gcc-patches wrote:
>> Now that I'm writing the above text and rereading the
>> pointer-interconvertibility definition, I think my first_nonstatic_data_member_p
>> and fold_builtin_is_pointer_inverconvertible_with_class have one bug,
>> for unions the pointer inter-convertibility doesn't talk about std layout at
>> all, so I think I need to check for std_layout_type_p only for non-union
>> class types and accept any union, std_layout_type_p or not.  But when
>> recursing from a union type into anonymous structure type punt if the
>> anonymous structure type is not std_layout_type_p + add testcase coverage.
> 
> For this part, here is an incremental fix.  Tested on x86_64-linux.
> 
> It also shows that in the case (we're beyond the standard in this case
> because anonymous structures are not in the standard) of union with
> non-std-layout anonymous structure in it, in the case in the testcases like:
> struct D {};
> struct E { [[no_unique_address]] D e; };
> union Y { int a; struct : public E { short b; long c; }; long long d; };

We don't already reject an anonymous struct with bases?  I think we 
should do so, in fixup_anonymous_aggr.  We might even require anonymous 
structs to be standard-layout.

> the builtin will return false for &Y::b - while &Y::b is at offset zero,
> the anonymous structure is not std-layout and therefore the
> pointer-interconvertibility rules say pointers aren't interconvertible.
> But in case like:
> union Y2 { int a; struct : public E { int b; long c; }; long long d; };
> it will return true for &Y2::b - while the same applies, there is
> another union member with int type.  In theory when seeing the PTRMEM_CST
> we could still differentiate, &Y2::a is ok but &Y2::b is not.  But as soon
> as we have just an INTEGER_CST with OFFSET_TYPE or need to check it at
> runtime, all we know is that we have pointer to int data member in Y2
> at offset 0, and that is the same for &Y2::a and &Y2::b.

Yep.

I'm inclined not to handle this extension case specifically.

> 2021-07-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/101539
> 	* semantics.c (first_nonstatic_data_member_p): Don't recurse into
> 	non-std-layout non-union class types from union type.
> 	(fold_builtin_is_pointer_inverconvertible_with_class): Don't check
> 	std-layout type for union types.
> 
> 	* g++.dg/cpp2a/is-pointer-interconvertible-with-class1.C: Add
> 	tests for non-std-layout union type.
> 	* g++.dg/cpp2a/is-pointer-interconvertible-with-class2.C: Likewise.
> 	* g++.dg/cpp2a/is-pointer-interconvertible-with-class4.C: Add
> 	tests for non-std-layout anonymous class type in union.
> 	* g++.dg/cpp2a/is-pointer-interconvertible-with-class5.C: Likewise.
> 
> --- gcc/cp/semantics.c.jj	2021-07-28 23:06:38.665443459 +0200
> +++ gcc/cp/semantics.c	2021-07-29 15:44:30.659713391 +0200
> @@ -10631,7 +10631,9 @@ first_nonstatic_data_member_p (tree type
>   	  if (TREE_CODE (type) != UNION_TYPE)
>   	    return first_nonstatic_data_member_p (TREE_TYPE (field),
>   						  membertype);
> -	  if (first_nonstatic_data_member_p (TREE_TYPE (field), membertype))
> +	  if ((TREE_CODE (TREE_TYPE (field)) == UNION_TYPE
> +	       || std_layout_type_p (TREE_TYPE (field)))
> +	      && first_nonstatic_data_member_p (TREE_TYPE (field), membertype))
>   	    return true;
>   	}
>         else if (TREE_CODE (type) != UNION_TYPE)
> @@ -10677,7 +10679,8 @@ fold_builtin_is_pointer_inverconvertible
>     if (!complete_type_or_else (basetype, NULL_TREE))
>       return boolean_false_node;
>   
> -  if (!std_layout_type_p (basetype))
> +  if (TREE_CODE (basetype) != UNION_TYPE
> +      && !std_layout_type_p (basetype))
>       return boolean_false_node;
>   
>     if (!first_nonstatic_data_member_p (basetype, membertype))
> --- gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class1.C.jj	2021-07-28 23:06:38.667443431 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class1.C	2021-07-29 15:46:55.809743808 +0200
> @@ -28,6 +28,7 @@ union U { int a; double b; long long c;
>   struct V { union { int a; long b; }; int c; };
>   union X { int a; union { short b; long c; }; long long d; };
>   struct Y { void foo () {} };
> +union Z { int a; private: int b; protected: int c; public: int d; };
>   
>   static_assert (std::is_pointer_interconvertible_with_class (&B::b));
>   static_assert (!std::is_pointer_interconvertible_with_class (&B::b2));
> @@ -60,3 +61,5 @@ static_assert (std::is_pointer_interconv
>   static_assert (std::is_pointer_interconvertible_with_class (&X::d));
>   static_assert (!std::is_pointer_interconvertible_with_class ((int B::*) nullptr));
>   static_assert (!std::is_pointer_interconvertible_with_class (&Y::foo));
> +static_assert (std::is_pointer_interconvertible_with_class (&Z::a));
> +static_assert (std::is_pointer_interconvertible_with_class (&Z::d));
> --- gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class2.C.jj	2021-07-28 23:06:38.667443431 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class2.C	2021-07-29 15:48:33.075423974 +0200
> @@ -28,6 +28,7 @@ union U { int a; double b; long long c;
>   struct V { union { int a; long b; }; int c; };
>   union X { int a; union { short b; long c; }; long long d; };
>   struct Y { void foo () {} };
> +union Z { int a; private: int b; protected: int c; public: int d; };
>   
>   int
>   main ()
> @@ -125,4 +126,10 @@ main ()
>     auto t31 = &Y::foo;
>     if (std::is_pointer_interconvertible_with_class (t31))
>       __builtin_abort ();
> +  auto t32 = &Z::a;
> +  if (!std::is_pointer_interconvertible_with_class (t32))
> +    __builtin_abort ();
> +  auto t33 = &Z::d;
> +  if (!std::is_pointer_interconvertible_with_class (t33))
> +    __builtin_abort ();
>   }
> --- gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class4.C.jj	2021-07-28 23:06:38.667443431 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class4.C	2021-07-29 15:56:54.340622140 +0200
> @@ -14,6 +14,9 @@ is_pointer_interconvertible_with_class (
>   
>   struct W { struct { int a; long b; }; int c; };
>   union X { int a; struct { short b; long c; }; long long d; };
> +struct D {};
> +struct E { [[no_unique_address]] D e; };
> +union Y { int a; struct : public E { short b; long c; }; long long d; };
>   
>   static_assert (std::is_pointer_interconvertible_with_class (&W::a));
>   static_assert (!std::is_pointer_interconvertible_with_class (&W::b));
> @@ -22,3 +25,7 @@ static_assert (std::is_pointer_interconv
>   static_assert (std::is_pointer_interconvertible_with_class (&X::b));
>   static_assert (!std::is_pointer_interconvertible_with_class (&X::c));
>   static_assert (std::is_pointer_interconvertible_with_class (&X::d));
> +static_assert (std::is_pointer_interconvertible_with_class (&Y::a));
> +static_assert (!std::is_pointer_interconvertible_with_class (&Y::b));
> +static_assert (!std::is_pointer_interconvertible_with_class (&Y::c));
> +static_assert (std::is_pointer_interconvertible_with_class (&Y::d));
> --- gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class5.C.jj	2021-07-28 23:06:38.667443431 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/is-pointer-interconvertible-with-class5.C	2021-07-29 15:57:10.829398399 +0200
> @@ -14,6 +14,9 @@ is_pointer_interconvertible_with_class (
>   
>   struct W { struct { int a; long b; }; int c; };
>   union X { int a; struct { short b; long c; }; long long d; };
> +struct D {};
> +struct E { [[no_unique_address]] D e; };
> +union Y { int a; struct : public E { short b; long c; }; long long d; };
>   
>   int
>   main ()
> @@ -39,4 +42,16 @@ main ()
>     auto t7 = &X::d;
>     if (!std::is_pointer_interconvertible_with_class (t7))
>       __builtin_abort ();
> +  auto t8 = &Y::a;
> +  if (!std::is_pointer_interconvertible_with_class (t8))
> +    __builtin_abort ();
> +  auto t9 = &Y::b;
> +  if (std::is_pointer_interconvertible_with_class (t9))
> +    __builtin_abort ();
> +  auto t10 = &Y::c;
> +  if (std::is_pointer_interconvertible_with_class (t10))
> +    __builtin_abort ();
> +  auto t11 = &Y::d;
> +  if (!std::is_pointer_interconvertible_with_class (t11))
> +    __builtin_abort ();
>   }
> 
> 	Jakub
>

On Fri, Jul 30, 2021 at 01:10:33AM -0400, Jason Merrill wrote:
> On 7/29/21 10:21 AM, Jakub Jelinek wrote:
> > On Thu, Jul 29, 2021 at 09:50:10AM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > Now that I'm writing the above text and rereading the
> > > pointer-interconvertibility definition, I think my first_nonstatic_data_member_p
> > > and fold_builtin_is_pointer_inverconvertible_with_class have one bug,
> > > for unions the pointer inter-convertibility doesn't talk about std layout at
> > > all, so I think I need to check for std_layout_type_p only for non-union
> > > class types and accept any union, std_layout_type_p or not.  But when
> > > recursing from a union type into anonymous structure type punt if the
> > > anonymous structure type is not std_layout_type_p + add testcase coverage.
> > 
> > For this part, here is an incremental fix.  Tested on x86_64-linux.
> > 
> > It also shows that in the case (we're beyond the standard in this case
> > because anonymous structures are not in the standard) of union with
> > non-std-layout anonymous structure in it, in the case in the testcases like:
> > struct D {};
> > struct E { [[no_unique_address]] D e; };
> > union Y { int a; struct : public E { short b; long c; }; long long d; };
> 
> We don't already reject an anonymous struct with bases?  I think we should
> do so, in fixup_anonymous_aggr.  We might even require anonymous structs to
> be standard-layout.

Apparently not, the above is accepted.  I was looking for an example of
non-stdlayout anon aggregate in union and my first try (mixing
private/public members) has been rejected.

> I'm inclined not to handle this extension case specifically.

You mean not to even recurse into anonymous structures in the function,
or something else?

	Jakub

On Fri, Jul 30, 2021 at 01:10:33AM -0400, Jason Merrill wrote:
> > It also shows that in the case (we're beyond the standard in this case
> > because anonymous structures are not in the standard) of union with
> > non-std-layout anonymous structure in it, in the case in the testcases like:
> > struct D {};
> > struct E { [[no_unique_address]] D e; };
> > union Y { int a; struct : public E { short b; long c; }; long long d; };
> 
> We don't already reject an anonymous struct with bases?  I think we should
> do so, in fixup_anonymous_aggr.  We might even require anonymous structs to
> be standard-layout.

Not having base classes seems reasonable requirement for the anonymous
structures, after all, I couldn't find a way to refer to the members
in the base class - &Y::e is rejected with the above.
But standard layout means that even all the non-static members of the struct
need to be standard-layout, that seems an unnecessary requirement for
anon structures to me.

	Jakub