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

  On Wed, Sep 29, 2021 at 03:38:45PM -0400, Jason Merrill wrote:
> 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.

So something like this?
spaceship-synth8.C (apparently added by me, so how valid/invalid it is is
unclear) now doesn't ICE anymore, but without the change I've put there
is now rejected because std::strong_ordering::less etc. aren't found.
Previously when we were synthesizing it we did that before the FIELD_DECLs
for bases have been added, so those weren't compared, but now they actually
are compared.
After fixing the incomplete std::strong_ordering spaceship-synth8.C is now
accepted, but I'm afraid I'm getting lost in this - clang++ rejects that
testcase instead complaining that D has <=> operator, but has it pure virtual.
And, when maybe_instantiate_noexcept tries to find out if the defaulted
method would be implicitly deleted or not, when it does so before the class
is complete, seems it can check whether there are errors when comparing the
direct members of the class, but not errors about bases...

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

	PR c++/102490
	* cp-tree.h (build_comparison_op): Declare.
	* method.c (comp_info): Remove defining member.
	(comp_info::comp_info): Remove complain argument, don't initialize
	defining.
	(build_comparison_op): No longer static.  Add defining argument.
	Adjust comp_info construction.  Use defining instead of info.defining.
	Assert that if defining, ctype is a complete type.
	(synthesize_method, maybe_explain_implicit_delete,
	explain_implicit_non_constexpr): Adjust build_comparison_op callers.
	* class.c (check_bases_and_members): Don't call defaulted_late_check
	for sfk_comparison.
	(finish_struct_1): Call it here instead after class has been completed.
	* pt.c (maybe_instantiate_noexcept): For sfk_comparison of still
	incomplete classes, call build_comparison_op in non-defining mode
	instead of calling synthesize_method.

	* g++.dg/cpp2a/spaceship-synth8.C (std::strong_ordering): Provide more
	complete definition.
	(std::strong_ordering::less, std::strong_ordering::equal,
	std::strong_ordering::greater): Define.
	* g++.dg/cpp2a/spaceship-eq11.C: New test.
	* g++.dg/cpp2a/spaceship-eq12.C: New test.



	Jakub
  

On 9/30/21 13:24, Jakub Jelinek wrote:
> On Wed, Sep 29, 2021 at 03:38:45PM -0400, Jason Merrill wrote:
>> 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.
> 
> So something like this?

Much like, thanks.

> spaceship-synth8.C (apparently added by me, so how valid/invalid it is is
> unclear) now doesn't ICE anymore, but without the change I've put there
> is now rejected because std::strong_ordering::less etc. aren't found.
> Previously when we were synthesizing it we did that before the FIELD_DECLs
> for bases have been added, so those weren't compared, but now they actually
> are compared.

Ah, good to have that fixed.

> After fixing the incomplete std::strong_ordering spaceship-synth8.C is now
> accepted, but I'm afraid I'm getting lost in this - clang++ rejects that
> testcase instead complaining that D has <=> operator, but has it pure virtual.

Ah, I think we need to add LOOKUP_NO_VIRTUAL to the flags variable, as 
we do in do_build_copy_assign.  I suppose it wouldn't hurt to add 
LOOKUP_DEFAULTED as well.

One more comment in the patch below.

> And, when maybe_instantiate_noexcept tries to find out if the defaulted
> method would be implicitly deleted or not, when it does so before the class
> is complete, seems it can check whether there are errors when comparing the
> direct members of the class, but not errors about bases...
> 
> 2021-09-30  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/102490
> 	* cp-tree.h (build_comparison_op): Declare.
> 	* method.c (comp_info): Remove defining member.
> 	(comp_info::comp_info): Remove complain argument, don't initialize
> 	defining.
> 	(build_comparison_op): No longer static.  Add defining argument.
> 	Adjust comp_info construction.  Use defining instead of info.defining.
> 	Assert that if defining, ctype is a complete type.
> 	(synthesize_method, maybe_explain_implicit_delete,
> 	explain_implicit_non_constexpr): Adjust build_comparison_op callers.
> 	* class.c (check_bases_and_members): Don't call defaulted_late_check
> 	for sfk_comparison.
> 	(finish_struct_1): Call it here instead after class has been completed.
> 	* pt.c (maybe_instantiate_noexcept): For sfk_comparison of still
> 	incomplete classes, call build_comparison_op in non-defining mode
> 	instead of calling synthesize_method.
> 
> 	* g++.dg/cpp2a/spaceship-synth8.C (std::strong_ordering): Provide more
> 	complete definition.
> 	(std::strong_ordering::less, std::strong_ordering::equal,
> 	std::strong_ordering::greater): Define.
> 	* g++.dg/cpp2a/spaceship-eq11.C: New test.
> 	* g++.dg/cpp2a/spaceship-eq12.C: New test.
> 
> --- gcc/cp/cp-tree.h.jj	2021-09-18 09:44:31.728743713 +0200
> +++ gcc/cp/cp-tree.h	2021-09-30 18:39:10.416847290 +0200
> @@ -7012,6 +7012,7 @@ extern bool maybe_explain_implicit_delet
>   extern void explain_implicit_non_constexpr	(tree);
>   extern bool deduce_inheriting_ctor		(tree);
>   extern bool decl_remember_implicit_trigger_p	(tree);
> +extern void build_comparison_op			(tree, bool, tsubst_flags_t);
>   extern void synthesize_method			(tree);
>   extern tree lazily_declare_fn			(special_function_kind,
>   						 tree);
> --- gcc/cp/method.c.jj	2021-09-30 09:22:46.323918164 +0200
> +++ gcc/cp/method.c	2021-09-30 18:51:14.510744549 +0200
> @@ -1288,21 +1288,16 @@ struct comp_info
>   {
>     tree fndecl;
>     location_t loc;
> -  bool defining;
>     bool first_time;
>     bool constexp;
>     bool was_constexp;
>     bool noex;
>   
> -  comp_info (tree fndecl, tsubst_flags_t &complain)
> +  comp_info (tree fndecl)
>       : fndecl (fndecl)
>     {
>       loc = DECL_SOURCE_LOCATION (fndecl);
>   
> -    /* We only have tf_error set when we're called from
> -       explain_invalid_constexpr_fn or maybe_explain_implicit_delete.  */
> -    defining = !(complain & tf_error);
> -
>       first_time = DECL_MAYBE_DELETED (fndecl);
>       DECL_MAYBE_DELETED (fndecl) = false;
>   
> @@ -1364,12 +1359,12 @@ struct comp_info
>      to use synthesize_method at the earliest opportunity and bail out if the
>      function ends up being deleted.  */
>   
> -static void
> -build_comparison_op (tree fndecl, tsubst_flags_t complain)
> +void
> +build_comparison_op (tree fndecl, bool defining, tsubst_flags_t complain)
>   {
> -  comp_info info (fndecl, complain);
> +  comp_info info (fndecl);
>   
> -  if (!info.defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
> +  if (!defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
>       return;
>   
>     int flags = LOOKUP_NORMAL;
> @@ -1384,6 +1379,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 (!defining || COMPLETE_TYPE_P (ctype));
>   
>     iloc_sentinel ils (info.loc);
>   
> @@ -1399,7 +1395,7 @@ build_comparison_op (tree fndecl, tsubst
>       }
>   
>     tree compound_stmt = NULL_TREE;
> -  if (info.defining)
> +  if (defining)
>       compound_stmt = begin_compound_stmt (0);
>     else
>       ++cp_unevaluated_operand;
> @@ -1474,8 +1470,8 @@ build_comparison_op (tree fndecl, tsubst
>   		break;
>   	      tree idx;
>   	      /* [1] array, no loop needed, just add [0] ARRAY_REF.
> -		 Similarly if !info.defining.  */
> -	      if (integer_zerop (maxval) || !info.defining)
> +		 Similarly if !defining.  */
> +	      if (integer_zerop (maxval) || !defining)
>   		idx = size_zero_node;
>   	      /* Some other array, will need runtime loop.  */
>   	      else
> @@ -1584,7 +1580,7 @@ build_comparison_op (tree fndecl, tsubst
>   	  tree comp = comps[i];
>   	  tree eq, retval = NULL_TREE, if_ = NULL_TREE;
>   	  tree loop_indexes = NULL_TREE;
> -	  if (info.defining)
> +	  if (defining)
>   	    {
>   	      if (TREE_CODE (comp) == TREE_LIST)
>   		{
> @@ -1632,7 +1628,7 @@ build_comparison_op (tree fndecl, tsubst
>   		comp = build_static_cast (input_location, rettype, comp,
>   					  complain);
>   	      info.check (comp);
> -	      if (info.defining)
> +	      if (defining)
>   		{
>   		  tree var = create_temporary_var (rettype);
>   		  pushdecl (var);
> @@ -1645,7 +1641,7 @@ build_comparison_op (tree fndecl, tsubst
>   	    }
>   	  tree ceq = contextual_conv_bool (eq, complain);
>   	  info.check (ceq);
> -	  if (info.defining)
> +	  if (defining)
>   	    {
>   	      finish_if_stmt_cond (ceq, if_);
>   	      finish_then_clause (if_);
> @@ -1658,7 +1654,7 @@ build_comparison_op (tree fndecl, tsubst
>   		finish_for_stmt (TREE_VALUE (loop_index));
>   	    }
>   	}
> -      if (info.defining)
> +      if (defining)
>   	{
>   	  tree val;
>   	  if (code == EQ_EXPR)
> @@ -1679,7 +1675,7 @@ build_comparison_op (tree fndecl, tsubst
>   				NULL_TREE, NULL, complain);
>         comp = contextual_conv_bool (comp, complain);
>         info.check (comp);
> -      if (info.defining)
> +      if (defining)
>   	{
>   	  tree neg = build1 (TRUTH_NOT_EXPR, boolean_type_node, comp);
>   	  finish_return_stmt (neg);
> @@ -1692,12 +1688,12 @@ build_comparison_op (tree fndecl, tsubst
>         tree comp2 = build_new_op (info.loc, code, flags, comp, integer_zero_node,
>   				 NULL_TREE, NULL, complain);
>         info.check (comp2);
> -      if (info.defining)
> +      if (defining)
>   	finish_return_stmt (comp2);
>       }
>   
>    out:
> -  if (info.defining)
> +  if (defining)
>       finish_compound_stmt (compound_stmt);
>     else
>       --cp_unevaluated_operand;
> @@ -1776,7 +1772,7 @@ synthesize_method (tree fndecl)
>     else if (sfk == sfk_comparison)
>       {
>         /* Pass tf_none so the function is just deleted if there's a problem.  */
> -      build_comparison_op (fndecl, tf_none);
> +      build_comparison_op (fndecl, true, tf_none);
>         need_body = false;
>       }
>   
> @@ -2747,7 +2743,7 @@ maybe_explain_implicit_delete (tree decl
>   	  inform (DECL_SOURCE_LOCATION (decl),
>   		  "%q#D is implicitly deleted because the default "
>   		  "definition would be ill-formed:", decl);
> -	  build_comparison_op (decl, tf_warning_or_error);
> +	  build_comparison_op (decl, false, tf_warning_or_error);
>   	}
>         else if (!informed)
>   	{
> @@ -2808,7 +2804,7 @@ explain_implicit_non_constexpr (tree dec
>     if (sfk == sfk_comparison)
>       {
>         DECL_DECLARED_CONSTEXPR_P (decl) = true;
> -      build_comparison_op (decl, tf_warning_or_error);
> +      build_comparison_op (decl, false, tf_warning_or_error);
>         DECL_DECLARED_CONSTEXPR_P (decl) = false;
>       }
>     else
> --- gcc/cp/class.c.jj	2021-09-30 09:22:46.299918499 +0200
> +++ gcc/cp/class.c	2021-09-30 18:40:52.330425343 +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;
> +      {
> +	/* Synthesize 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/cp/pt.c.jj	2021-09-29 10:07:41.815881043 +0200
> +++ gcc/cp/pt.c	2021-09-30 18:48:09.091331517 +0200
> @@ -25765,6 +25765,25 @@ maybe_instantiate_noexcept (tree fn, tsu
>   	/* We're in start_preparsed_function, keep going.  */
>   	return true;
>   
> +      if (special_function_p (fn) == sfk_comparison)
> +	{
> +	  tree lhs = DECL_ARGUMENTS (fn);
> +	  if (is_this_parameter (lhs))
> +	    lhs = cp_build_fold_indirect_ref (lhs);
> +	  else
> +	    lhs = convert_from_reference (lhs);
> +	  tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
> +	  /* If the comparison type is still incomplete, don't synthesize the
> +	     method, just see if it is not implicitly deleted.  */
> +	  if (!COMPLETE_TYPE_P (ctype))
> +	    {
> +	      push_deferring_access_checks (dk_no_deferred);
> +	      build_comparison_op (fn, false, tf_none);
> +	      pop_deferring_access_checks ();
> +	      return !DECL_MAYBE_DELETED (fn);
> +	    }
> +	}
> +
>         ++function_depth;
>         synthesize_method (fn);
>         --function_depth;

Let's factor this (from the added code to here) into a 
maybe_synthesize_method in method.c.  That way build_comparison_op can 
stay static.

> --- gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C.jj	2020-07-28 15:39:10.012756173 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C	2021-09-30 19:02:22.107430284 +0200
> @@ -1,7 +1,18 @@
>   // PR c++/94907
>   // { dg-do compile { target c++20 } }
>   
> -namespace std { struct strong_ordering { }; }
> +namespace std { struct strong_ordering {
> +  int _v;
> +  constexpr strong_ordering (int v) :_v(v) {}
> +  constexpr operator int (void) const { return _v; }
> +  static const strong_ordering less;
> +  static const strong_ordering equal;
> +  static const strong_ordering greater;
> +};
> +constexpr strong_ordering strong_ordering::less = -1;
> +constexpr strong_ordering strong_ordering::equal = 0;
> +constexpr strong_ordering strong_ordering::greater = 1;
> +}
>   
>   struct E;
>   struct D {
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj	2021-09-30 18:34:22.897858916 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C	2021-09-30 18:34:22.897858916 +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-30 18:34:22.897858916 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C	2021-09-30 18:34:22.897858916 +0200
> @@ -0,0 +1,5 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +// { dg-options "-O2" }
> +
> +#include "spaceship-eq11.C"
> 
> 
> 	Jakub
>