[v2] c++: Fix up synthetization of defaulted comparison operators on classes with bitfields [PR102490]

  On Tue, Sep 28, 2021 at 03:33:35PM -0400, Jason Merrill wrote:
> > > According to the function comment for defaulted_late_check, won't
> > > COMPLETE_TYPE_P (ctx) always be false here?
> 
> Not for a function defaulted outside the class.
> 
> > If so, I wonder if we could get away with moving this entire fragment
> > from defaulted_late_check to finish_struct_1 instead of calling
> > defaulted_late_check from finish_struct_1.
> 
> The comment in check_bases_and_members says that we call it there so that
> it's before we clone [cd]tors.  Probably better to leave the call there for
> other functions, just skip it for comparisons.

So like this instead then?  Just tested with dg.exp=*spaceship* so far.

2021-09-28  Jakub Jelinek  <jakub@redhat.com>

	PR c++/102490
	* method.c (defaulted_late_check): Don't synthetize constexpr
	defaulted comparisons.
	(finish_struct_1): Synthetize constexpr defaulted comparisons here
	after layout_class_type.

	* g++.dg/cpp2a/spaceship-eq11.C: New test.
	* g++.dg/cpp2a/spaceship-eq12.C: New test.



	Jakub
  

On Tue, Sep 28, 2021 at 10:34:39PM +0200, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Sep 28, 2021 at 03:33:35PM -0400, Jason Merrill wrote:
> > > > According to the function comment for defaulted_late_check, won't
> > > > COMPLETE_TYPE_P (ctx) always be false here?
> > 
> > Not for a function defaulted outside the class.
> > 
> > > If so, I wonder if we could get away with moving this entire fragment
> > > from defaulted_late_check to finish_struct_1 instead of calling
> > > defaulted_late_check from finish_struct_1.
> > 
> > The comment in check_bases_and_members says that we call it there so that
> > it's before we clone [cd]tors.  Probably better to leave the call there for
> > other functions, just skip it for comparisons.
> 
> So like this instead then?  Just tested with dg.exp=*spaceship* so far.
> 
> 2021-09-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/102490
> 	* method.c (defaulted_late_check): Don't synthetize constexpr
> 	defaulted comparisons.
> 	(finish_struct_1): Synthetize constexpr defaulted comparisons here
> 	after layout_class_type.
> 
> 	* g++.dg/cpp2a/spaceship-eq11.C: New test.
> 	* g++.dg/cpp2a/spaceship-eq12.C: New test.

Also successfully bootstrapped/regtested on x86_64-linux and i686-linux.

	Jakub
  

On 9/28/21 16:34, Jakub Jelinek wrote:
> On Tue, Sep 28, 2021 at 03:33:35PM -0400, Jason Merrill wrote:
>>>> According to the function comment for defaulted_late_check, won't
>>>> COMPLETE_TYPE_P (ctx) always be false here?
>>
>> Not for a function defaulted outside the class.
>>
>>> If so, I wonder if we could get away with moving this entire fragment
>>> from defaulted_late_check to finish_struct_1 instead of calling
>>> defaulted_late_check from finish_struct_1.
>>
>> The comment in check_bases_and_members says that we call it there so that
>> it's before we clone [cd]tors.  Probably better to leave the call there for
>> other functions, just skip it for comparisons.
> 
> So like this instead then?  Just tested with dg.exp=*spaceship* so far.

I was thinking to keep the checking/synthesis in defaulted_late_check, 
just change when it's called.

That is, not to change defaulted_late_check at all, to keep the 
finish_struct_1 change you had in your first patch, and to change 
check_bases_and_members to not call defaulted_late_check for comparisons.

We probably also want to assert COMPLETE_TYPE_P in build_comparison_op, 
at least when info.defining.

BTW, the word is "synthesize".

> 2021-09-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/102490
> 	* method.c (defaulted_late_check): Don't synthetize constexpr
> 	defaulted comparisons.
> 	(finish_struct_1): Synthetize constexpr defaulted comparisons here
> 	after layout_class_type.
> 
> 	* g++.dg/cpp2a/spaceship-eq11.C: New test.
> 	* g++.dg/cpp2a/spaceship-eq12.C: New test.
> 
> --- gcc/cp/method.c.jj	2021-09-28 11:34:10.165412477 +0200
> +++ gcc/cp/method.c	2021-09-28 22:28:23.637981709 +0200
> @@ -3158,18 +3158,7 @@ defaulted_late_check (tree fn)
>     special_function_kind kind = special_function_p (fn);
>   
>     if (kind == sfk_comparison)
> -    {
> -      /* If the function was declared constexpr, check that the definition
> -	 qualifies.  Otherwise we can define the function lazily.  */
> -      if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn))
> -	{
> -	  /* Prevent GC.  */
> -	  function_depth++;
> -	  synthesize_method (fn);
> -	  function_depth--;
> -	}
> -      return;
> -    }
> +    return;
>   
>     bool fn_const_p = (copy_fn_p (fn) == 2);
>     tree implicit_fn = implicitly_declare_fn (kind, ctx, fn_const_p,
> --- gcc/cp/class.c.jj	2021-09-28 11:34:10.096413431 +0200
> +++ gcc/cp/class.c	2021-09-28 22:29:59.072669058 +0200
> @@ -7467,7 +7467,21 @@ finish_struct_1 (tree t)
>        for any static member objects of the type we're working on.  */
>     for (x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
>       if (DECL_DECLARES_FUNCTION_P (x))
> -      DECL_IN_AGGR_P (x) = false;
> +      {
> +	/* Synthetize constexpr defaulted comparisons.  */
> +	if (!DECL_ARTIFICIAL (x)
> +	    && DECL_DEFAULTED_IN_CLASS_P (x)
> +	    && special_function_p (x) == sfk_comparison
> +	    && DECL_DECLARED_CONSTEXPR_P (x)
> +	    && !DECL_INITIAL (x))
> +	  {
> +	    /* Prevent GC.  */
> +	    function_depth++;
> +	    synthesize_method (x);
> +	    function_depth--;
> +	  }
> +	DECL_IN_AGGR_P (x) = false;
> +      }
>       else if (VAR_P (x) && TREE_STATIC (x)
>   	     && TREE_TYPE (x) != error_mark_node
>   	     && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (x)), t))
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj	2021-09-28 22:27:40.524574708 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C	2021-09-28 22:27:40.524574708 +0200
> @@ -0,0 +1,43 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +
> +struct A
> +{
> +  unsigned char a : 1;
> +  unsigned char b : 1;
> +  constexpr bool operator== (const A &) const = default;
> +};
> +
> +struct B
> +{
> +  unsigned char a : 8;
> +  int : 0;
> +  unsigned char b : 7;
> +  constexpr bool operator== (const B &) const = default;
> +};
> +
> +struct C
> +{
> +  unsigned char a : 3;
> +  unsigned char b : 1;
> +  constexpr bool operator== (const C &) const = default;
> +};
> +
> +void
> +foo (C &x, int y)
> +{
> +  x.b = y;
> +}
> +
> +int
> +main ()
> +{
> +  A a{}, b{};
> +  B c{}, d{};
> +  C e{}, f{};
> +  a.b = 1;
> +  d.b = 1;
> +  foo (e, 0);
> +  foo (f, 1);
> +  return a == b || c == d || e == f;
> +}
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C.jj	2021-09-28 22:27:40.524574708 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C	2021-09-28 22:27:40.524574708 +0200
> @@ -0,0 +1,5 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +// { dg-options "-O2" }
> +
> +#include "spaceship-eq11.C"
> 
> 
> 	Jakub
>
  

On Wed, Sep 29, 2021 at 02:14:36PM -0400, Jason Merrill wrote:
> On 9/28/21 16:34, Jakub Jelinek wrote:
> > On Tue, Sep 28, 2021 at 03:33:35PM -0400, Jason Merrill wrote:
> > > > > According to the function comment for defaulted_late_check, won't
> > > > > COMPLETE_TYPE_P (ctx) always be false here?
> > > 
> > > Not for a function defaulted outside the class.
> > > 
> > > > If so, I wonder if we could get away with moving this entire fragment
> > > > from defaulted_late_check to finish_struct_1 instead of calling
> > > > defaulted_late_check from finish_struct_1.
> > > 
> > > The comment in check_bases_and_members says that we call it there so that
> > > it's before we clone [cd]tors.  Probably better to leave the call there for
> > > other functions, just skip it for comparisons.
> > 
> > So like this instead then?  Just tested with dg.exp=*spaceship* so far.
> 
> I was thinking to keep the checking/synthesis in defaulted_late_check, just
> change when it's called.
> 
> That is, not to change defaulted_late_check at all, to keep the
> finish_struct_1 change you had in your first patch, and to change
> check_bases_and_members to not call defaulted_late_check for comparisons.
>
> We probably also want to assert COMPLETE_TYPE_P in build_comparison_op, at
> least when info.defining.

I've tried that, but the assertion in build_comparison_op ICEs on
spaceship-synth8.C testcase:
#0  fancy_abort (file=0x2d75b3c "../../gcc/cp/method.c", line=1387, function=0x2d761c3 "build_comparison_op") at ../../gcc/diagnostic.c:1982
#1  0x0000000000c8dad0 in build_comparison_op (fndecl=<function_decl 0x7fffea195600 operator<=>>, complain=0) at ../../gcc/cp/method.c:1387
#2  0x0000000000c8fa7c in synthesize_method (fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/method.c:1780
#3  0x0000000000e0ac55 in maybe_instantiate_noexcept (fn=<function_decl 0x7fffea195600 operator<=>>, complain=3) at ../../gcc/cp/pt.c:25769
#4  0x0000000000e33672 in maybe_check_overriding_exception_spec (overrider=<function_decl 0x7fffea195600 operator<=>>, basefn=<function_decl 0x7fffea195500 operator<=>>)
    at ../../gcc/cp/search.c:1897
#5  0x0000000000e33f5e in check_final_overrider (overrider=<function_decl 0x7fffea195600 operator<=>>, basefn=<function_decl 0x7fffea195500 operator<=>>)
    at ../../gcc/cp/search.c:2015
#6  0x0000000000e35365 in look_for_overrides_r (type=<record_type 0x7fffea191dc8 D>, fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/search.c:2183
#7  0x0000000000e34e60 in look_for_overrides (type=<record_type 0x7fffea191d20 E>, fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/search.c:2127
#8  0x0000000000afc3ba in check_for_override (decl=<function_decl 0x7fffea195600 operator<=>>, ctype=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:2934
#9  0x0000000000b047c2 in check_methods (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:4720
#10 0x0000000000b0d25c in check_bases_and_members (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:5990
#11 0x0000000000b141de in finish_struct_1 (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:7373
#12 0x0000000000b15f16 in finish_struct (t=<record_type 0x7fffea191d20 E>, attributes=<tree 0x0>) at ../../gcc/cp/class.c:7676

Do you think there is any hope to defer even that, or is that something
that has to be done before layout_class_type?

--- gcc/cp/method.c.jj	2021-09-29 10:07:28.791062239 +0200
+++ gcc/cp/method.c	2021-09-29 20:24:14.229131851 +0200
@@ -1384,6 +1384,7 @@ build_comparison_op (tree fndecl, tsubst
     lhs = convert_from_reference (lhs);
   rhs = convert_from_reference (rhs);
   tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
+  gcc_assert (COMPLETE_TYPE_P (ctype));
 
   iloc_sentinel ils (info.loc);
 
--- gcc/cp/class.c.jj	2021-09-29 10:07:28.745062879 +0200
+++ gcc/cp/class.c	2021-09-29 20:23:13.117984089 +0200
@@ -6133,7 +6133,8 @@ check_bases_and_members (tree t)
 		 no longer ill-formed, it is defined as deleted instead.  */
 	      DECL_DELETED_FN (fn) = true;
 	  }
-	defaulted_late_check (fn);
+	if (special_function_p (fn) != sfk_comparison)
+	  defaulted_late_check (fn);
       }
 
   if (LAMBDA_TYPE_P (t))
@@ -7467,7 +7468,14 @@ finish_struct_1 (tree t)
      for any static member objects of the type we're working on.  */
   for (x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
     if (DECL_DECLARES_FUNCTION_P (x))
-      DECL_IN_AGGR_P (x) = false;
+      {
+	/* Synthetize constexpr defaulted comparisons.  */
+	if (!DECL_ARTIFICIAL (x)
+	    && DECL_DEFAULTED_IN_CLASS_P (x)
+	    && special_function_p (x) == sfk_comparison)
+	  defaulted_late_check (x);
+	DECL_IN_AGGR_P (x) = false;
+      }
     else if (VAR_P (x) && TREE_STATIC (x)
 	     && TREE_TYPE (x) != error_mark_node
 	     && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (x)), t))
--- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj	2021-09-29 20:21:49.861145165 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C	2021-09-29 20:21:49.861145165 +0200
@@ -0,0 +1,43 @@
+// PR c++/102490
+// { dg-do run { target c++20 } }
+
+struct A
+{
+  unsigned char a : 1;
+  unsigned char b : 1;
+  constexpr bool operator== (const A &) const = default;
+};
+
+struct B
+{
+  unsigned char a : 8;
+  int : 0;
+  unsigned char b : 7;
+  constexpr bool operator== (const B &) const = default;
+};
+
+struct C
+{
+  unsigned char a : 3;
+  unsigned char b : 1;
+  constexpr bool operator== (const C &) const = default;
+};
+
+void
+foo (C &x, int y)
+{
+  x.b = y;
+}
+
+int
+main ()
+{
+  A a{}, b{};
+  B c{}, d{};
+  C e{}, f{};
+  a.b = 1;
+  d.b = 1;
+  foo (e, 0);
+  foo (f, 1);
+  return a == b || c == d || e == f;
+}
--- gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C.jj	2021-09-29 20:21:49.861145165 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C	2021-09-29 20:21:49.861145165 +0200
@@ -0,0 +1,5 @@
+// PR c++/102490
+// { dg-do run { target c++20 } }
+// { dg-options "-O2" }
+
+#include "spaceship-eq11.C"


	Jakub
  

On 9/29/21 15:21, Jakub Jelinek wrote:
> On Wed, Sep 29, 2021 at 02:14:36PM -0400, Jason Merrill wrote:
>> On 9/28/21 16:34, Jakub Jelinek wrote:
>>> On Tue, Sep 28, 2021 at 03:33:35PM -0400, Jason Merrill wrote:
>>>>>> According to the function comment for defaulted_late_check, won't
>>>>>> COMPLETE_TYPE_P (ctx) always be false here?
>>>>
>>>> Not for a function defaulted outside the class.
>>>>
>>>>> If so, I wonder if we could get away with moving this entire fragment
>>>>> from defaulted_late_check to finish_struct_1 instead of calling
>>>>> defaulted_late_check from finish_struct_1.
>>>>
>>>> The comment in check_bases_and_members says that we call it there so that
>>>> it's before we clone [cd]tors.  Probably better to leave the call there for
>>>> other functions, just skip it for comparisons.
>>>
>>> So like this instead then?  Just tested with dg.exp=*spaceship* so far.
>>
>> I was thinking to keep the checking/synthesis in defaulted_late_check, just
>> change when it's called.
>>
>> That is, not to change defaulted_late_check at all, to keep the
>> finish_struct_1 change you had in your first patch, and to change
>> check_bases_and_members to not call defaulted_late_check for comparisons.
>>
>> We probably also want to assert COMPLETE_TYPE_P in build_comparison_op, at
>> least when info.defining.
> 
> I've tried that, but the assertion in build_comparison_op ICEs on
> spaceship-synth8.C testcase:
> #0  fancy_abort (file=0x2d75b3c "../../gcc/cp/method.c", line=1387, function=0x2d761c3 "build_comparison_op") at ../../gcc/diagnostic.c:1982
> #1  0x0000000000c8dad0 in build_comparison_op (fndecl=<function_decl 0x7fffea195600 operator<=>>, complain=0) at ../../gcc/cp/method.c:1387
> #2  0x0000000000c8fa7c in synthesize_method (fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/method.c:1780
> #3  0x0000000000e0ac55 in maybe_instantiate_noexcept (fn=<function_decl 0x7fffea195600 operator<=>>, complain=3) at ../../gcc/cp/pt.c:25769
> #4  0x0000000000e33672 in maybe_check_overriding_exception_spec (overrider=<function_decl 0x7fffea195600 operator<=>>, basefn=<function_decl 0x7fffea195500 operator<=>>)
>      at ../../gcc/cp/search.c:1897
> #5  0x0000000000e33f5e in check_final_overrider (overrider=<function_decl 0x7fffea195600 operator<=>>, basefn=<function_decl 0x7fffea195500 operator<=>>)
>      at ../../gcc/cp/search.c:2015
> #6  0x0000000000e35365 in look_for_overrides_r (type=<record_type 0x7fffea191dc8 D>, fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/search.c:2183
> #7  0x0000000000e34e60 in look_for_overrides (type=<record_type 0x7fffea191d20 E>, fndecl=<function_decl 0x7fffea195600 operator<=>>) at ../../gcc/cp/search.c:2127
> #8  0x0000000000afc3ba in check_for_override (decl=<function_decl 0x7fffea195600 operator<=>>, ctype=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:2934
> #9  0x0000000000b047c2 in check_methods (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:4720
> #10 0x0000000000b0d25c in check_bases_and_members (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:5990
> #11 0x0000000000b141de in finish_struct_1 (t=<record_type 0x7fffea191d20 E>) at ../../gcc/cp/class.c:7373
> #12 0x0000000000b15f16 in finish_struct (t=<record_type 0x7fffea191d20 E>, attributes=<tree 0x0>) at ../../gcc/cp/class.c:7676
> 
> Do you think there is any hope to defer even that, or is that something
> that has to be done before layout_class_type?

It ought to be possible to defer check_final_overrider, but it sounds 
awkward.

Or maybe_instantiate_noexcept could use the non-defining path in 
build_comparison_op.

Maybe we want a maybe_synthesize_method that actually builds the 
function if the type is complete, or takes the non-defining path if not.

> --- gcc/cp/method.c.jj	2021-09-29 10:07:28.791062239 +0200
> +++ gcc/cp/method.c	2021-09-29 20:24:14.229131851 +0200
> @@ -1384,6 +1384,7 @@ build_comparison_op (tree fndecl, tsubst
>       lhs = convert_from_reference (lhs);
>     rhs = convert_from_reference (rhs);
>     tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
> +  gcc_assert (COMPLETE_TYPE_P (ctype));
>     iloc_sentinel ils (info.loc);
>   
> --- gcc/cp/class.c.jj	2021-09-29 10:07:28.745062879 +0200
> +++ gcc/cp/class.c	2021-09-29 20:23:13.117984089 +0200
> @@ -6133,7 +6133,8 @@ check_bases_and_members (tree t)
>   		 no longer ill-formed, it is defined as deleted instead.  */
>   	      DECL_DELETED_FN (fn) = true;
>   	  }
> -	defaulted_late_check (fn);
> +	if (special_function_p (fn) != sfk_comparison)
> +	  defaulted_late_check (fn);
>         }
>   
>     if (LAMBDA_TYPE_P (t))
> @@ -7467,7 +7468,14 @@ finish_struct_1 (tree t)
>        for any static member objects of the type we're working on.  */
>     for (x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
>       if (DECL_DECLARES_FUNCTION_P (x))
> -      DECL_IN_AGGR_P (x) = false;
> +      {
> +	/* Synthetize constexpr defaulted comparisons.  */

"Synthesize"

> +	if (!DECL_ARTIFICIAL (x)
> +	    && DECL_DEFAULTED_IN_CLASS_P (x)
> +	    && special_function_p (x) == sfk_comparison)
> +	  defaulted_late_check (x);
> +	DECL_IN_AGGR_P (x) = false;
> +      }
>       else if (VAR_P (x) && TREE_STATIC (x)
>   	     && TREE_TYPE (x) != error_mark_node
>   	     && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (x)), t))
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj	2021-09-29 20:21:49.861145165 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C	2021-09-29 20:21:49.861145165 +0200
> @@ -0,0 +1,43 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +
> +struct A
> +{
> +  unsigned char a : 1;
> +  unsigned char b : 1;
> +  constexpr bool operator== (const A &) const = default;
> +};
> +
> +struct B
> +{
> +  unsigned char a : 8;
> +  int : 0;
> +  unsigned char b : 7;
> +  constexpr bool operator== (const B &) const = default;
> +};
> +
> +struct C
> +{
> +  unsigned char a : 3;
> +  unsigned char b : 1;
> +  constexpr bool operator== (const C &) const = default;
> +};
> +
> +void
> +foo (C &x, int y)
> +{
> +  x.b = y;
> +}
> +
> +int
> +main ()
> +{
> +  A a{}, b{};
> +  B c{}, d{};
> +  C e{}, f{};
> +  a.b = 1;
> +  d.b = 1;
> +  foo (e, 0);
> +  foo (f, 1);
> +  return a == b || c == d || e == f;
> +}
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C.jj	2021-09-29 20:21:49.861145165 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C	2021-09-29 20:21:49.861145165 +0200
> @@ -0,0 +1,5 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +// { dg-options "-O2" }
> +
> +#include "spaceship-eq11.C"
> 
> 
> 	Jakub
>